Re: How to force a push to succeed?
On Tue, Aug 22, 2017 at 05:55:25PM -0400, Jeffrey Walton wrote: > I tested some changes that lead to a dead end. The changes need to be > removed. The changes were added in 7 commits. > > I went back in time to the point before the changes: > > $ git reset --hard HEAD~7 > HEAD is now at 559fc3b Fix benchmark selection code (GH #464) > > When I attempted to push: > > $ git push > Username for 'https://github.com': noloader > To https://github.com/noloader/cryptopp.git > ! [rejected]master -> master (non-fast-forward) > > I tried to commit, but Git claims there's nothing to add: > > $ git commit > On branch master > Your branch is behind 'origin/master' by 7 commits, and can be > fast-forwarded. > > Commit seems to be the wrong command as Git appears to be trying to do > something I don't want. > > How do I force the push to succeed? > > Thanks in advance. By actually doing a force push: git push --force-with-lease origin master But note that this can interfere with others working on the same repository, so be carefull when you use a force push. Additionally, I can recommend using separate branches for things that might turn up as dead ends (or even for every development). That way, you can just throw away the branch if you want to discard it. Hope this helps, Kevin.
sequencer status
Hi, I've created a small tool to display the current sequencer status. It mimics what Magit does to display what was done and what is left to do. As someone who often rebase large series of patches (also works with am, revert, cherry-pick), I really needed the feature and use this daily. It's available here: https://github.com/nmorey/git-sequencer-status SUSE and Fedora packages are available here: https://build.opensuse.org/package/show/home:NMoreyChaisemartin:git-tools/git-sequencer-status It's not necessary very robust yet. Most of the time I use simple rebase so there are a lot of untested corner cases. Here is an example output: $ sequencer-status # Interactive rebase: master onto rebase_conflict exec true pick e54d993 Fourth commit exec true *pick b064629 Third commit exec true pick 174c933 Second commit onto 773cb23 Alternate second Two questions: - Could this be a candidate for contrib/ ? - Would it be interesting to add the relevant code to sequencer.c so that all sequencer based commands could have a --status option ? Nicolas
[BUG] rebase -i with only empty commits
Hi, On 08/23/2017 01:08 AM, Stefan Beller wrote: > The editor opened proposing the following instruction sheet, > which in my opinion is buggy: > > pick 1234 some commit > exec make > pick 2345 another commit > exec make > pick 3456 third commit > # pick 4567 empty commit > exec make > pick 5678 yet another commit > exec make This reminds me of another bug I stumbled over recently regarding empty commits. Do this: # repo preparation: git init :> file1 git add file1 git commit -m "add file1" :> file2 git add file2 git commit -m "add file2" # the bug: git checkout -b to-be-rebased master^ git commit --allow-empty -m "empty commit" git rebase -i master It says "Nothing to do". Unsurprisingly, the problem persists when you apply other empty commits: git commit --allow-empty -m "another empty commit" git rebase -i master Adding a "real" commit solves the problem: :>file3 git add file3 git commit -m "add file3" Adding further empty commits is no problem: git commit --allow-empty -m "yet another empty commit" So the problem seems to be that rebase -i (like rebase without -i) considers "empty commits" as commits to be ignored. However, when using rebase -i one expects that you can include the empty commit... Also, the behavior is odd. When I only have empty commits, a "git rebase master" works as expected like a "git reset --hard master" but "git rebase -i" does nothing. The expected behavior would be that the editor shows up with a git-rebase-todo like: # pick 3d0f6c49 empty commit # pick bbbc5941 another empty commit noop Thanks Stephan
[PATCH/RFC] push: anonymize URL in error output
Commits 47abd85 (fetch: Strip usernames from url's before storing them,
2009-04-17) and later 882d49c (push: anonymize URL in status output,
2016-07-14) made fetch and push strip the authentication part of the
remote URLs when used in the merge-commit messages or status outputs.
The URLs that are part of the error messages were not anonymized.
A commonly used pattern for storing artifacts from a build server in a
remote repository utilizes a "secure" environment variable with
credentials to embed them in the URL and execute a push. Given enough
runs, an intermittent network failure will cause a push to fail, leaving
a non-anonymized URL in the build log.
To prevent that, reuse the same anonymizing function to scrub
credentials from URL in the push error output.
Signed-off-by: Ivan Vyshnevskyi
---
This is my first attempt to propose a patch, sorry if I did something wrong!
I've tested my changes on Travis CI, and the build is green [1].
Not sure how much of the background should be included in the commit message.
The "commonly used pattern" I mention could be found in the myriad of
online tutorials and looks something like this:
git push -fq https://$git_cr...@github.com/$REPO_SLUG
Note, that a lot of developers mistakenly assume that '--quiet' ('-q') will
suppress all output. Sometimes, they would redirect standard output to
/dev/null, without realizing that errors are printed out to stderr.
I didn't mention this in the commit, but another typical offender is a tool that
calls 'git push' as part of its execution. This is a spectrum that starts with
shell scripts, includes simple one-task apps in Python or Ruby, and ends with
the plugins for JavaScript, Java, Groovy, and Scala build tools. (I'd like to
avoid shaming their authors publicly, but could send you a few examples
privately.)
I gathered the data by going through build logs of popular open source projects
(and projects of their contributors) hosted on GitHub and built by Travis CI.
I found about 2.3k unique credentials (but only about nine hundred were active
at the time), and more than a half of those were exposed by a failed push. See
the advisory from Travis CI [2] for results of their own scan.
While the issue is public for several months now and addressed on Travis CI,
I keep finding build logs with credentials on the internet. So I think it's
worth fixing in the git itself.
[1]: https://travis-ci.org/sainaen/git/builds/267180560
[2]: https://blog.travis-ci.com/2017-05-08-security-advisory
builtin/push.c | 2 +-
t/t5541-http-push-smart.sh | 18 ++
2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/builtin/push.c b/builtin/push.c
index 03846e837..59f3bc975 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -336,7 +336,7 @@ static int push_with_options(struct transport *transport,
int flags)
err = transport_push(transport, refspec_nr, refspec, flags,
&reject_reasons);
if (err != 0)
- error(_("failed to push some refs to '%s'"), transport->url);
+ error(_("failed to push some refs to '%s'"),
transport_anonymize_url(transport->url));
err |= transport_disconnect(transport);
if (!err)
diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index d38bf3247..0b6fb6252 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -377,5 +377,23 @@ test_expect_success 'push status output scrubs password' '
grep "^To $HTTPD_URL/smart/test_repo.git" status
'
+cat >"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update" act &&
+ test_i18ncmp exp act
+'
+rm -f "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update"
+
stop_httpd
test_done
--
2.14.1
Re: git send-email Cc with cruft not working as expected
Stefan Beller writes: > +cc people from that thread > > On Tue, Aug 22, 2017 at 4:30 PM, Jacob Keller wrote: >> On Tue, Aug 22, 2017 at 4:18 PM, Stefan Beller wrote: >>> On Tue, Aug 22, 2017 at 4:15 PM, Jacob Keller >>> wrote: Hi, I recently found an issue with git-send-email where it does not properly remove the cruft of an email address when sending using a Cc: line. The specific example is with a commit containing the following Cc line, Cc: sta...@vger.kernel.org # 4.10+ >>> >>> Please see and discuss at >>> https://public-inbox.org/git/20170216174924.GB2625@localhost/ >> >> I read that thread, and it addressed the problem of >> >> Cc: # 4.10+ >> >> but did not fix this case without the <> around the email address. Indeed. It detects garbage as "everything after >". I feel really sorry that we need so many iterations to get back to a correct behavior :-(. >> Additionally I just discovered that the behavior here changes pretty >> drastically if you have Email::Validate installed, now it splits the >> address into multiple things: (I'm assuming you mean Email::Address, there's also Email::Valid but I don't think it would modify the behavior) Hmm, I think we reached the point where we should just stop using Email::Address. Patch series follows and should address both points. -- Matthieu Moy http://matthieu-moy.fr
[RFC PATCH 1/2] send-email: fix garbage removal after address
This is a followup over 9d33439 (send-email: only allow one address
per body tag, 2017-02-20). The first iteration did allow writting
Cc: # garbage
but did so by matching the regex ([^>]*>?), i.e. stop after the first
instance of '>'. However, it did not properly deal with
Cc: f...@example.com # garbage
Fix this using a new function strip_garbage_one_address, which does
essentially what the old ([^>]*>?) was doing, but dealing with more
corner-cases. Since we've allowed
Cc: "Foo # Bar"
in previous versions, it makes sense to continue allowing it (but we
still remove any garbage after it). OTOH, when an address is given
without quoting, we just take the first word and ignore everything
after.
Signed-off-by: Matthieu Moy
---
Also available as: https://github.com/git/git/pull/398
git-send-email.perl | 26 --
t/t9001-send-email.sh | 4
2 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index fa6526986e..33a69ffe5d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1089,6 +1089,26 @@ sub sanitize_address {
}
+sub strip_garbage_one_address {
+ my ($addr) = @_;
+ chomp $addr;
+ if ($addr =~ /^(("[^"]*"|[^"<]*)? *<[^>]*>).*/) {
+ # "Foo Bar" [possibly garbage here]
+ # Foo Bar [possibly garbage here]
+ return $1;
+ }
+ if ($addr =~ /^(<[^>]*>).*/) {
+ # [possibly garbage here]
+ # if garbage contains other addresses, they are ignored.
+ return $1;
+ }
+ if ($addr =~ /^([^"#,\s]*)/) {
+ # address without quoting: remove anything after the address
+ return $1;
+ }
+ return $addr;
+}
+
sub sanitize_address_list {
return (map { sanitize_address($_) } @_);
}
@@ -1590,10 +1610,12 @@ foreach my $t (@files) {
# Now parse the message body
while(<$fh>) {
$message .= $_;
- if (/^(Signed-off-by|Cc): ([^>]*>?)/i) {
+ if (/^(Signed-off-by|Cc): (.*)/i) {
chomp;
my ($what, $c) = ($1, $2);
- chomp $c;
+ # strip garbage for the address we'll use:
+ $c = strip_garbage_one_address($c);
+ # sanitize a bit more to decide whether to suppress the
address:
my $sc = sanitize_address($c);
if ($sc eq $sender) {
next if ($suppress_cc{'self'});
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index d1e4e8ad19..f30980895c 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -148,6 +148,8 @@ cat >expected-cc <<\EOF
!t...@example.com!
!th...@example.com!
!f...@example.com!
+!f...@example.com!
+!s...@example.com!
EOF
"
@@ -161,6 +163,8 @@ test_expect_success $PREREQ 'cc trailer with various
syntax' '
Cc: # trailing comments are ignored
Cc: , one address per line
Cc: "Some # Body" [ ]
+ Cc: f...@example.com # not@example.com
+ Cc: s...@example.com, not.se...@example.com
EOF
clean_fake_sendmail &&
git send-email -1 --to=recipi...@example.com \
--
2.14.0.rc0.dirty
[RFC PATCH 2/2] send-email: don't use Mail::Address, even if available
Using Mail::Address made sense when we didn't have a proper parser. We
now have a reasonable address parser, and using Mail::Address
_if available_ causes much more trouble than it gives benefits:
* Developers typically test one version, not both.
* Users may not be aware that installing Mail::Address will change the
behavior. They may complain about the behavior in one case without
knowing that Mail::Address is involved.
* Having this optional Mail::Address makes it tempting to anwser "please
install Mail::Address" to users instead of fixing our own code. We've
reached the stage where bugs in our parser should be fixed, not worked
around.
Signed-off-by: Matthieu Moy
---
git-send-email.perl | 7 +--
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 33a69ffe5d..2208dcc213 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -155,7 +155,6 @@ sub format_2822_time {
}
my $have_email_valid = eval { require Email::Valid; 1 };
-my $have_mail_address = eval { require Mail::Address; 1 };
my $smtp;
my $auth;
my $num_sent = 0;
@@ -490,11 +489,7 @@ my ($repoauthor, $repocommitter);
($repocommitter) = Git::ident_person(@repo, 'committer');
sub parse_address_line {
- if ($have_mail_address) {
- return map { $_->format } Mail::Address->parse($_[0]);
- } else {
- return Git::parse_mailboxes($_[0]);
- }
+ return Git::parse_mailboxes($_[0]);
}
sub split_addrs {
--
2.14.0.rc0.dirty
Re: [PATCH/RFC] push: anonymize URL in error output
> On 23 Aug 2017, at 11:49, Ivan Vyshnevskyi wrote:
>
> Commits 47abd85 (fetch: Strip usernames from url's before storing them,
> 2009-04-17) and later 882d49c (push: anonymize URL in status output,
> 2016-07-14) made fetch and push strip the authentication part of the
> remote URLs when used in the merge-commit messages or status outputs.
> The URLs that are part of the error messages were not anonymized.
>
> A commonly used pattern for storing artifacts from a build server in a
> remote repository utilizes a "secure" environment variable with
> credentials to embed them in the URL and execute a push. Given enough
> runs, an intermittent network failure will cause a push to fail, leaving
> a non-anonymized URL in the build log.
>
> To prevent that, reuse the same anonymizing function to scrub
> credentials from URL in the push error output.
>
> Signed-off-by: Ivan Vyshnevskyi
> ---
>
> This is my first attempt to propose a patch, sorry if I did something wrong!
>
> I've tested my changes on Travis CI, and the build is green [1].
>
> Not sure how much of the background should be included in the commit message.
> The "commonly used pattern" I mention could be found in the myriad of
> online tutorials and looks something like this:
>
>git push -fq https://$git_cr...@github.com/$REPO_SLUG
>
> Note, that a lot of developers mistakenly assume that '--quiet' ('-q') will
> suppress all output. Sometimes, they would redirect standard output to
> /dev/null, without realizing that errors are printed out to stderr.
>
> I didn't mention this in the commit, but another typical offender is a tool
> that
> calls 'git push' as part of its execution. This is a spectrum that starts with
> shell scripts, includes simple one-task apps in Python or Ruby, and ends with
> the plugins for JavaScript, Java, Groovy, and Scala build tools. (I'd like to
> avoid shaming their authors publicly, but could send you a few examples
> privately.)
>
> I gathered the data by going through build logs of popular open source
> projects
> (and projects of their contributors) hosted on GitHub and built by Travis CI.
> I found about 2.3k unique credentials (but only about nine hundred were active
> at the time), and more than a half of those were exposed by a failed push. See
> the advisory from Travis CI [2] for results of their own scan.
>
> While the issue is public for several months now and addressed on Travis CI,
> I keep finding build logs with credentials on the internet. So I think it's
> worth fixing in the git itself.
>
> [1]: https://travis-ci.org/sainaen/git/builds/267180560
> [2]: https://blog.travis-ci.com/2017-05-08-security-advisory
>
This sounds very reasonable to me! Thanks for the contribution!
I wonder if we should even extend this. Consider this case:
$ git push https://lars:secret@server/org/repo1
remote: Invalid username or password.
fatal: Authentication failed for 'https://lars:secret@server/org/repo1/'
I might not have valid credentials for repo1 but my credentials could
very well be valid for repo2.
- Lars
> builtin/push.c | 2 +-
> t/t5541-http-push-smart.sh | 18 ++
> 2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/push.c b/builtin/push.c
> index 03846e837..59f3bc975 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -336,7 +336,7 @@ static int push_with_options(struct transport *transport,
> int flags)
> err = transport_push(transport, refspec_nr, refspec, flags,
>&reject_reasons);
> if (err != 0)
> - error(_("failed to push some refs to '%s'"), transport->url);
> + error(_("failed to push some refs to '%s'"),
> transport_anonymize_url(transport->url));
>
> err |= transport_disconnect(transport);
> if (!err)
> diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
> index d38bf3247..0b6fb6252 100755
> --- a/t/t5541-http-push-smart.sh
> +++ b/t/t5541-http-push-smart.sh
> @@ -377,5 +377,23 @@ test_expect_success 'push status output scrubs password'
> '
> grep "^To $HTTPD_URL/smart/test_repo.git" status
> '
>
> +cat >"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update" < +#!/bin/sh
> +exit 1
> +EOF
> +chmod a+x "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update"
> +
> +cat >exp < +error: failed to push some refs to '$HTTPD_URL/smart/test_repo.git'
> +EOF
> +
> +test_expect_success 'failed push status output scrubs password' '
> + cd "$ROOT_PATH"/test_repo_clone &&
> + test_must_fail git push "$HTTPD_URL_USER_PASS/smart/test_repo.git"
> +HEAD:scrub_err 2>stderr &&
> + grep "^error: failed to push some refs" stderr >act &&
> + test_i18ncmp exp act
> +'
> +rm -f "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update"
> +
> stop_httpd
> test_done
> --
> 2.14.1
>
[PATCH v3 1/4] Documentation/git-merge: explain --continue
Currently, 'git merge --continue' is mentioned but not explained. Explain it. Signed-off-by: Michael J Gruber --- Documentation/git-merge.txt | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt index 6b308ab6d0..615e6bacde 100644 --- a/Documentation/git-merge.txt +++ b/Documentation/git-merge.txt @@ -288,7 +288,10 @@ After seeing a conflict, you can do two things: * Resolve the conflicts. Git will mark the conflicts in the working tree. Edit the files into shape and - 'git add' them to the index. Use 'git commit' to seal the deal. + 'git add' them to the index. Use 'git commit' or + 'git merge --continue' to seal the deal. The latter command + checks whether there is a (interrupted) merge in progress + before calling 'git commit'. You can work through the conflict with a number of tools: -- 2.14.1.426.g4352aa77a5
[PATCH v3 4/4] merge: save merge state earlier
If the `git merge` process is killed while waiting for the editor to
finish, the merge state is lost but the prepared merge msg and tree is kept.
So, a subsequent `git commit` creates a squashed merge even when the
user asked for proper merge commit originally.
Demonstrate the problem with a test crafted after the in t7502. The test
requires EXECKEEPSPID (thus does not run under MINGW).
Save the merge state earlier (in the non-squash case) so that it does
not get lost. This makes the test pass.
Reported-by: hIpPy
Signed-off-by: Michael J Gruber
---
builtin/merge.c | 2 ++
t/t7600-merge.sh | 15 +++
2 files changed, 17 insertions(+)
diff --git a/builtin/merge.c b/builtin/merge.c
index db3335b3bf..2d5c45a904 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -758,6 +758,7 @@ N_("Please enter a commit message to explain why this merge
is necessary,\n"
"Lines starting with '%c' will be ignored, and an empty message aborts\n"
"the commit.\n");
+static void write_merge_heads(struct commit_list *);
static void prepare_to_commit(struct commit_list *remoteheads)
{
struct strbuf msg = STRBUF_INIT;
@@ -769,6 +770,7 @@ static void prepare_to_commit(struct commit_list
*remoteheads)
strbuf_commented_addf(&msg, _(merge_editor_comment),
comment_line_char);
if (signoff)
append_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);
+ write_merge_heads(remoteheads);
write_file_buf(git_path_merge_msg(), msg.buf, msg.len);
if (run_commit_hook(0 < option_edit, get_index_file(),
"prepare-commit-msg",
git_path_merge_msg(), "merge", NULL))
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 2ebda509ac..80194b79f9 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -774,4 +774,19 @@ test_expect_success 'merge can be completed with
--continue' '
verify_parents $c0 $c1
'
+write_script .git/FAKE_EDITOR <> .git/FAKE_EDITOR
+ GIT_EDITOR=.git/FAKE_EDITOR
+ export GIT_EDITOR
+ exec git merge --no-ff --edit c1'\'' &&
+ git merge --continue &&
+ verify_parents $c0 $c1
+'
+
test_done
--
2.14.1.426.g4352aa77a5
[PATCH v3 2/4] merge: clarify call chain
prepare_to_commit() cannot be reached in the non-squash case:
It is called by merge_trivial() and finish_automerge() only, but the
calls to the latter are somewhat hard to track:
If option_commit is not set, the code in cmd_merge() uses a fake
conflict return code (ret=1) to avoid writing the tree, which also
avoids setting automerge_was_ok (just as in the proper ret==1 case), so
that finish_automerge() is not called.
To ensure that no code change breaks that assumption, safe-guard
prepare_to_commit() by a BUG() statement.
Suggested-by: junio
Signed-off-by: Michael J Gruber
---
builtin/merge.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/builtin/merge.c b/builtin/merge.c
index cc57052993..dafec80fa9 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -763,6 +763,8 @@ static void prepare_to_commit(struct commit_list
*remoteheads)
struct strbuf msg = STRBUF_INIT;
strbuf_addbuf(&msg, &merge_msg);
strbuf_addch(&msg, '\n');
+ if (squash)
+ BUG("the control must not reach here under --squash");
if (0 < option_edit)
strbuf_commented_addf(&msg, _(merge_editor_comment),
comment_line_char);
if (signoff)
--
2.14.1.426.g4352aa77a5
[PATCH v3 0/4] Keep merge during kills
Compared to the 3-item v2: 1/4 == 1/3 2/4 is new as per Junio's suggestion: clarify the call-chain leading up to prepare_to_commit() 3/4 == 2/3 with amended subject 4/4 is 3/3 rebased, squash-if removed Michael J Gruber (4): Documentation/git-merge: explain --continue merge: clarify call chain merge: split write_merge_state in two merge: save merge state earlier Documentation/git-merge.txt | 5 - builtin/merge.c | 15 --- t/t7600-merge.sh| 15 +++ 3 files changed, 31 insertions(+), 4 deletions(-) -- 2.14.1.426.g4352aa77a5
[PATCH v3 3/4] merge: split write_merge_state in two
write_merge_state() writes out the merge heads, mode, and msg. But we
may want to write out heads, mode without the msg. So, split out heads
(+mode) into a separate function write_merge_heads() that is called by
write_merge_state().
No funtional change so far, except when these non-atomic writes are
interrupted: we write heads-mode-msg now when we used to write
heads-msg-mode.
Signed-off-by: Michael J Gruber
---
builtin/merge.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index dafec80fa9..db3335b3bf 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -910,7 +910,7 @@ static int setup_with_upstream(const char ***argv)
return i;
}
-static void write_merge_state(struct commit_list *remoteheads)
+static void write_merge_heads(struct commit_list *remoteheads)
{
struct commit_list *j;
struct strbuf buf = STRBUF_INIT;
@@ -926,8 +926,6 @@ static void write_merge_state(struct commit_list
*remoteheads)
strbuf_addf(&buf, "%s\n", oid_to_hex(oid));
}
write_file_buf(git_path_merge_head(), buf.buf, buf.len);
- strbuf_addch(&merge_msg, '\n');
- write_file_buf(git_path_merge_msg(), merge_msg.buf, merge_msg.len);
strbuf_reset(&buf);
if (fast_forward == FF_NO)
@@ -935,6 +933,13 @@ static void write_merge_state(struct commit_list
*remoteheads)
write_file_buf(git_path_merge_mode(), buf.buf, buf.len);
}
+static void write_merge_state(struct commit_list *remoteheads)
+{
+ write_merge_heads(remoteheads);
+ strbuf_addch(&merge_msg, '\n');
+ write_file_buf(git_path_merge_msg(), merge_msg.buf, merge_msg.len);
+}
+
static int default_edit_option(void)
{
static const char name[] = "GIT_MERGE_AUTOEDIT";
--
2.14.1.426.g4352aa77a5
[PATCH v4 00/16] Fix git-gc losing objects in multi worktree
"git gc" when used in multiple worktrees ignore some per-worktree references: object references in the index, HEAD and reflog. This series fixes it by making the revision walker include these from all worktrees by default (and the series is basically split in three parts in the same order). There's a couple more cleanups in refs.c. Luckily it does not conflict with anything in 'pu'. Compared to v3 [1], the largest change is supporting multi worktree in the reflog iterator. The merge iterator is now used (Micheal was right all along). [1] https://public-inbox.org/git/20170419110145.5086-1-pclo...@gmail.com/ Nguyễn Thái Ngọc Duy (16): revision.h: new flag in struct rev_info wrt. worktree-related refs refs.c: use is_dir_sep() in resolve_gitlink_ref() revision.c: refactor add_index_objects_to_pending() revision.c: --indexed-objects add objects from all worktrees refs.c: refactor get_submodule_ref_store(), share common free block refs: move submodule slash stripping code to get_submodule_ref_store refs: add refs_head_ref() revision.c: use refs_for_each*() instead of for_each_*_submodule() refs.c: move for_each_remote_ref_submodule() to submodule.c refs: remove dead for_each_*_submodule() revision.c: --all adds HEAD from all worktrees files-backend: make reflog iterator go through per-worktree reflog revision.c: --reflog add HEAD reflog from all worktrees rev-list: expose and document --single-worktree refs.c: remove fallback-to-main-store code get_submodule_ref_store() refs.c: reindent get_submodule_ref_store() Documentation/rev-list-options.txt| 8 ++ Documentation/technical/api-ref-iteration.txt | 7 +- reachable.c | 2 + refs.c| 110 ++--- refs.h| 20 +--- refs/files-backend.c | 59 +--- revision.c| 131 +- revision.h| 1 + submodule.c | 9 ++ t/t1407-worktree-ref-store.sh | 30 ++ t/t5304-prune.sh | 37 worktree.c| 22 + worktree.h| 8 ++ 13 files changed, 308 insertions(+), 136 deletions(-) -- 2.11.0.157.gd943d85
[PATCH v4 01/16] revision.h: new flag in struct rev_info wrt. worktree-related refs
The revision walker can walk through per-worktree refs like HEAD or
SHA-1 references in the index. These currently are from the current
worktree only. This new flag is added to change rev-list behavior in
this regard:
When single_worktree is set, only current worktree is considered. When
it is not set (which is the default), all worktrees are considered.
The default is chosen so because the two big components that rev-list
works with are object database (entirely shared between worktrees) and
refs (mostly shared). It makes sense that default behavior goes per-repo
too instead of per-worktree.
The flag will eventually be exposed as a rev-list argument with
documents. For now it stays internal until the new behavior is fully
implemented.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
revision.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/revision.h b/revision.h
index bc18487d6f..3a3d3e2cf8 100644
--- a/revision.h
+++ b/revision.h
@@ -96,6 +96,7 @@ struct rev_info {
topo_order:1,
simplify_merges:1,
simplify_by_decoration:1,
+ single_worktree:1,
tag_objects:1,
tree_objects:1,
blob_objects:1,
--
2.11.0.157.gd943d85
[PATCH v4 02/16] refs.c: use is_dir_sep() in resolve_gitlink_ref()
The "submodule" argument in this function is a path, which can have either '/' or '\\' as a separator. Use is_dir_sep() to support both. Noticed-by: Johannes Sixt Signed-off-by: Nguyễn Thái Ngọc Duy --- refs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/refs.c b/refs.c index 3d549a8970..dec899a57a 100644 --- a/refs.c +++ b/refs.c @@ -1507,7 +1507,7 @@ int resolve_gitlink_ref(const char *submodule, const char *refname, struct ref_store *refs; int flags; - while (len && submodule[len - 1] == '/') + while (len && is_dir_sep(submodule[len - 1])) len--; if (!len) -- 2.11.0.157.gd943d85
[PATCH v4 05/16] refs.c: refactor get_submodule_ref_store(), share common free block
Signed-off-by: Nguyễn Thái Ngọc Duy
---
refs.c | 17 ++---
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/refs.c b/refs.c
index dec899a57a..522c4ab74f 100644
--- a/refs.c
+++ b/refs.c
@@ -1636,7 +1636,6 @@ struct ref_store *get_submodule_ref_store(const char
*submodule)
{
struct strbuf submodule_sb = STRBUF_INIT;
struct ref_store *refs;
- int ret;
if (!submodule || !*submodule) {
/*
@@ -1648,19 +1647,14 @@ struct ref_store *get_submodule_ref_store(const char
*submodule)
refs = lookup_ref_store_map(&submodule_ref_stores, submodule);
if (refs)
- return refs;
+ goto done;
strbuf_addstr(&submodule_sb, submodule);
- ret = is_nonbare_repository_dir(&submodule_sb);
- strbuf_release(&submodule_sb);
- if (!ret)
- return NULL;
+ if (!is_nonbare_repository_dir(&submodule_sb))
+ goto done;
- ret = submodule_to_gitdir(&submodule_sb, submodule);
- if (ret) {
- strbuf_release(&submodule_sb);
- return NULL;
- }
+ if (submodule_to_gitdir(&submodule_sb, submodule))
+ goto done;
/* assume that add_submodule_odb() has been called */
refs = ref_store_init(submodule_sb.buf,
@@ -1668,6 +1662,7 @@ struct ref_store *get_submodule_ref_store(const char
*submodule)
register_ref_store_map(&submodule_ref_stores, "submodule",
refs, submodule);
+done:
strbuf_release(&submodule_sb);
return refs;
}
--
2.11.0.157.gd943d85
[PATCH v4 04/16] revision.c: --indexed-objects add objects from all worktrees
This is the result of single_worktree flag never being set (no way to up
until now). To get objects from current index only, set single_worktree.
The other add_index_objects_to_pending's caller is mark_reachable_objects()
(e.g. "git prune") which also mark objects from all indexes.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
revision.c | 21 +
t/t5304-prune.sh | 9 +
2 files changed, 30 insertions(+)
diff --git a/revision.c b/revision.c
index 6c46ad6c8a..f35cb49af5 100644
--- a/revision.c
+++ b/revision.c
@@ -19,6 +19,7 @@
#include "dir.h"
#include "cache-tree.h"
#include "bisect.h"
+#include "worktree.h"
volatile show_early_output_fn_t show_early_output;
@@ -1290,8 +1291,28 @@ static void do_add_index_objects_to_pending(struct
rev_info *revs,
void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
{
+ struct worktree **worktrees, **p;
+
read_cache();
do_add_index_objects_to_pending(revs, &the_index);
+
+ if (revs->single_worktree)
+ return;
+
+ worktrees = get_worktrees(0);
+ for (p = worktrees; *p; p++) {
+ struct worktree *wt = *p;
+ struct index_state istate = { NULL };
+
+ if (wt->is_current)
+ continue; /* current index already taken care of */
+
+ if (read_index_from(&istate,
+ worktree_git_path(wt, "index")) > 0)
+ do_add_index_objects_to_pending(revs, &istate);
+ discard_index(&istate);
+ }
+ free_worktrees(worktrees);
}
static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 133b5842b1..cba45c7be9 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -283,4 +283,13 @@ test_expect_success 'prune: handle alternate object
database' '
git -C B prune
'
+test_expect_success 'prune: handle index in multiple worktrees' '
+ git worktree add second-worktree &&
+ echo "new blob for second-worktree" >second-worktree/blob &&
+ git -C second-worktree add blob &&
+ git prune --expire=now &&
+ git -C second-worktree show :blob >actual &&
+ test_cmp second-worktree/blob actual
+'
+
test_done
--
2.11.0.157.gd943d85
[PATCH v4 03/16] revision.c: refactor add_index_objects_to_pending()
The core code is factored out and take 'struct index_state *' instead so
that we can reuse it to add objects from index files other than .git/index
in the next patch.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
revision.c | 18 --
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/revision.c b/revision.c
index aa3b946a8d..6c46ad6c8a 100644
--- a/revision.c
+++ b/revision.c
@@ -1262,13 +1262,13 @@ static void add_cache_tree(struct cache_tree *it,
struct rev_info *revs,
}
-void add_index_objects_to_pending(struct rev_info *revs, unsigned flags)
+static void do_add_index_objects_to_pending(struct rev_info *revs,
+ struct index_state *istate)
{
int i;
- read_cache();
- for (i = 0; i < active_nr; i++) {
- struct cache_entry *ce = active_cache[i];
+ for (i = 0; i < istate->cache_nr; i++) {
+ struct cache_entry *ce = istate->cache[i];
struct blob *blob;
if (S_ISGITLINK(ce->ce_mode))
@@ -1281,13 +1281,19 @@ void add_index_objects_to_pending(struct rev_info
*revs, unsigned flags)
ce->ce_mode, ce->name);
}
- if (active_cache_tree) {
+ if (istate->cache_tree) {
struct strbuf path = STRBUF_INIT;
- add_cache_tree(active_cache_tree, revs, &path);
+ add_cache_tree(istate->cache_tree, revs, &path);
strbuf_release(&path);
}
}
+void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
+{
+ read_cache();
+ do_add_index_objects_to_pending(revs, &the_index);
+}
+
static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
int exclude_parent)
{
--
2.11.0.157.gd943d85
[PATCH v4 06/16] refs: move submodule slash stripping code to get_submodule_ref_store
This is a better place that will benefit all submodule callers instead
of just resolve_gitlink_ref()
Signed-off-by: Nguyễn Thái Ngọc Duy
---
refs.c | 33 +
1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/refs.c b/refs.c
index 522c4ab74f..ea8e6b9f42 100644
--- a/refs.c
+++ b/refs.c
@@ -1503,25 +1503,10 @@ const char *resolve_ref_unsafe(const char *refname, int
resolve_flags,
int resolve_gitlink_ref(const char *submodule, const char *refname,
unsigned char *sha1)
{
- size_t len = strlen(submodule);
struct ref_store *refs;
int flags;
- while (len && is_dir_sep(submodule[len - 1]))
- len--;
-
- if (!len)
- return -1;
-
- if (submodule[len]) {
- /* We need to strip off one or more trailing slashes */
- char *stripped = xmemdupz(submodule, len);
-
- refs = get_submodule_ref_store(stripped);
- free(stripped);
- } else {
- refs = get_submodule_ref_store(submodule);
- }
+ refs = get_submodule_ref_store(submodule);
if (!refs)
return -1;
@@ -1636,6 +1621,16 @@ struct ref_store *get_submodule_ref_store(const char
*submodule)
{
struct strbuf submodule_sb = STRBUF_INIT;
struct ref_store *refs;
+ char *to_free = NULL;
+ size_t len;
+
+ if (submodule) {
+ len = strlen(submodule);
+ while (len && is_dir_sep(submodule[len - 1]))
+ len--;
+ if (!len)
+ return NULL;
+ }
if (!submodule || !*submodule) {
/*
@@ -1645,6 +1640,10 @@ struct ref_store *get_submodule_ref_store(const char
*submodule)
return get_main_ref_store();
}
+ if (submodule[len])
+ /* We need to strip off one or more trailing slashes */
+ submodule = to_free = xmemdupz(submodule, len);
+
refs = lookup_ref_store_map(&submodule_ref_stores, submodule);
if (refs)
goto done;
@@ -1664,6 +1663,8 @@ struct ref_store *get_submodule_ref_store(const char
*submodule)
done:
strbuf_release(&submodule_sb);
+ free(to_free);
+
return refs;
}
--
2.11.0.157.gd943d85
[PATCH v4 08/16] revision.c: use refs_for_each*() instead of for_each_*_submodule()
Signed-off-by: Nguyễn Thái Ngọc Duy
---
refs.c | 9 -
refs.h | 6 +++---
revision.c | 48
3 files changed, 39 insertions(+), 24 deletions(-)
diff --git a/refs.c b/refs.c
index b3a0a24469..cd61509bc8 100644
--- a/refs.c
+++ b/refs.c
@@ -1362,16 +1362,15 @@ int for_each_ref_in_submodule(const char *submodule,
const char *prefix,
prefix, fn, cb_data);
}
-int for_each_fullref_in_submodule(const char *submodule, const char *prefix,
- each_ref_fn fn, void *cb_data,
- unsigned int broken)
+int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix,
+each_ref_fn fn, void *cb_data,
+unsigned int broken)
{
unsigned int flag = 0;
if (broken)
flag = DO_FOR_EACH_INCLUDE_BROKEN;
- return do_for_each_ref(get_submodule_ref_store(submodule),
- prefix, fn, 0, flag, cb_data);
+ return do_for_each_ref(refs, prefix, fn, 0, flag, cb_data);
}
int for_each_replace_ref(each_ref_fn fn, void *cb_data)
diff --git a/refs.h b/refs.h
index 8073f8ab56..a8d6f33703 100644
--- a/refs.h
+++ b/refs.h
@@ -291,6 +291,9 @@ int refs_for_each_remote_ref(struct ref_store *refs,
int head_ref(each_ref_fn fn, void *cb_data);
int for_each_ref(each_ref_fn fn, void *cb_data);
int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data);
+int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix,
+each_ref_fn fn, void *cb_data,
+unsigned int broken);
int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data,
unsigned int broken);
int for_each_tag_ref(each_ref_fn fn, void *cb_data);
@@ -306,9 +309,6 @@ int for_each_ref_submodule(const char *submodule,
each_ref_fn fn, void *cb_data);
int for_each_ref_in_submodule(const char *submodule, const char *prefix,
each_ref_fn fn, void *cb_data);
-int for_each_fullref_in_submodule(const char *submodule, const char *prefix,
- each_ref_fn fn, void *cb_data,
- unsigned int broken);
int for_each_tag_ref_submodule(const char *submodule,
each_ref_fn fn, void *cb_data);
int for_each_branch_ref_submodule(const char *submodule,
diff --git a/revision.c b/revision.c
index f35cb49af5..8d04516266 100644
--- a/revision.c
+++ b/revision.c
@@ -1188,12 +1188,19 @@ void add_ref_exclusion(struct string_list
**ref_excludes_p, const char *exclude)
string_list_append(*ref_excludes_p, exclude);
}
-static void handle_refs(const char *submodule, struct rev_info *revs, unsigned
flags,
- int (*for_each)(const char *, each_ref_fn, void *))
+static void handle_refs(struct ref_store *refs,
+ struct rev_info *revs, unsigned flags,
+ int (*for_each)(struct ref_store *, each_ref_fn, void
*))
{
struct all_refs_cb cb;
+
+ if (!refs) {
+ /* this could happen with uninitialized submodules */
+ return;
+ }
+
init_all_refs_cb(&cb, revs, flags);
- for_each(submodule, handle_one_ref, &cb);
+ for_each(refs, handle_one_ref, &cb);
}
static void handle_one_reflog_commit(struct object_id *oid, void *cb_data)
@@ -2095,23 +2102,25 @@ void parse_revision_opt(struct rev_info *revs, struct
parse_opt_ctx_t *ctx,
ctx->argc -= n;
}
-static int for_each_bisect_ref(const char *submodule, each_ref_fn fn, void
*cb_data, const char *term) {
+static int for_each_bisect_ref(struct ref_store *refs, each_ref_fn fn,
+ void *cb_data, const char *term)
+{
struct strbuf bisect_refs = STRBUF_INIT;
int status;
strbuf_addf(&bisect_refs, "refs/bisect/%s", term);
- status = for_each_fullref_in_submodule(submodule, bisect_refs.buf, fn,
cb_data, 0);
+ status = refs_for_each_fullref_in(refs, bisect_refs.buf, fn, cb_data,
0);
strbuf_release(&bisect_refs);
return status;
}
-static int for_each_bad_bisect_ref(const char *submodule, each_ref_fn fn, void
*cb_data)
+static int for_each_bad_bisect_ref(struct ref_store *refs, each_ref_fn fn,
void *cb_data)
{
- return for_each_bisect_ref(submodule, fn, cb_data, term_bad);
+ return for_each_bisect_ref(refs, fn, cb_data, term_bad);
}
-static int for_each_good_bisect_ref(const char *submodule, each_ref_fn fn,
void *cb_data)
+static int for_each_good_bisect_ref(struct ref_store *refs, each_ref_fn fn,
void *cb_data)
{
- return for_each_bisect_ref(submodule, fn, cb_data, term_good);
+ return for_each_bisect_ref(refs, fn, cb_data, term_good);
}
static int handle_revision_pseudo_opt(const c
[PATCH v4 12/16] files-backend: make reflog iterator go through per-worktree reflog
refs/bisect is unfortunately per-worktree, so we need to look in
per-worktree logs/refs/bisect in addition to per-repo logs/refs. The
current iterator only goes through per-repo logs/refs.
Use merge iterator to walk two ref stores at the same time and pick
per-worktree refs from the right iterator.
PS. Note the unsorted order of for_each_reflog in the test. This is
supposed to be OK, for now. If we enforce order on for_each_reflog()
then some more work will be required.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
refs/files-backend.c | 59 +--
t/t1407-worktree-ref-store.sh | 30 ++
2 files changed, 75 insertions(+), 14 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 5cca55510b..d4d22882ef 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -106,15 +106,6 @@ static void files_reflog_path(struct files_ref_store *refs,
struct strbuf *sb,
const char *refname)
{
- if (!refname) {
- /*
-* FIXME: of course this is wrong in multi worktree
-* setting. To be fixed real soon.
-*/
- strbuf_addf(sb, "%s/logs", refs->gitcommondir);
- return;
- }
-
switch (ref_type(refname)) {
case REF_TYPE_PER_WORKTREE:
case REF_TYPE_PSEUDOREF:
@@ -2055,23 +2046,63 @@ static struct ref_iterator_vtable
files_reflog_iterator_vtable = {
files_reflog_iterator_abort
};
-static struct ref_iterator *files_reflog_iterator_begin(struct ref_store
*ref_store)
+static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,
+ const char *gitdir)
{
- struct files_ref_store *refs =
- files_downcast(ref_store, REF_STORE_READ,
- "reflog_iterator_begin");
struct files_reflog_iterator *iter = xcalloc(1, sizeof(*iter));
struct ref_iterator *ref_iterator = &iter->base;
struct strbuf sb = STRBUF_INIT;
base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable);
- files_reflog_path(refs, &sb, NULL);
+ strbuf_addf(&sb, "%s/logs", gitdir);
iter->dir_iterator = dir_iterator_begin(sb.buf);
iter->ref_store = ref_store;
strbuf_release(&sb);
+
return ref_iterator;
}
+static enum iterator_selection reflog_iterator_select(
+ struct ref_iterator *iter_worktree,
+ struct ref_iterator *iter_common,
+ void *cb_data)
+{
+ if (iter_worktree) {
+ /*
+* We're a bit loose here. We probably should ignore
+* common refs if they are accidentally added as
+* per-worktree refs.
+*/
+ return ITER_SELECT_0;
+ } else if (iter_common) {
+ if (ref_type(iter_common->refname) == REF_TYPE_NORMAL)
+ return ITER_SELECT_1;
+
+ /*
+* The main ref store may contain main worktree's
+* per-worktree refs, which should be ignored
+*/
+ return ITER_SKIP_1;
+ } else
+ return ITER_DONE;
+}
+
+static struct ref_iterator *files_reflog_iterator_begin(struct ref_store
*ref_store)
+{
+ struct files_ref_store *refs =
+ files_downcast(ref_store, REF_STORE_READ,
+ "reflog_iterator_begin");
+
+ if (!strcmp(refs->gitdir, refs->gitcommondir)) {
+ return reflog_iterator_begin(ref_store, refs->gitcommondir);
+ } else {
+ return merge_ref_iterator_begin(
+ reflog_iterator_begin(ref_store, refs->gitdir),
+ reflog_iterator_begin(ref_store, refs->gitcommondir),
+ reflog_iterator_select, refs);
+ }
+}
+
/*
* If update is a direct update of head_ref (the reference pointed to
* by HEAD), then add an extra REF_LOG_ONLY update for HEAD.
diff --git a/t/t1407-worktree-ref-store.sh b/t/t1407-worktree-ref-store.sh
index 5df06f3556..8842d0329f 100755
--- a/t/t1407-worktree-ref-store.sh
+++ b/t/t1407-worktree-ref-store.sh
@@ -49,4 +49,34 @@ test_expect_success 'create_symref(FOO, refs/heads/master)' '
test_cmp expected actual
'
+test_expect_success 'for_each_reflog()' '
+ echo $_z40 > .git/logs/PSEUDO-MAIN &&
+ mkdir -p .git/logs/refs/bisect &&
+ echo $_z40 > .git/logs/refs/bisect/random &&
+
+ echo $_z40 > .git/worktrees/wt/logs/PSEUDO-WT &&
+ mkdir -p .git/worktrees/wt/logs/refs/bisect &&
+ echo $_z40 > .git/worktrees/wt/logs/refs/bisect/wt-random &&
+
+ $RWT for-each-reflog | cut -c 42- | sort >actual &&
+ cat >expected <<-\EOF &&
+ HEAD 0x1
+ PSEUDO-WT 0x0
+ refs/bisect/wt-random 0x0
+ refs/heads/master 0x0
+ ref
[PATCH v4 13/16] revision.c: --reflog add HEAD reflog from all worktrees
Note that add_other_reflogs_to_pending() is a bit inefficient, since
it scans reflog for all refs of each worktree, including shared refs,
so the shared ref's reflog is scanned over and over again.
We could update refs API to pass "per-worktree only" flag to avoid
that. But long term we should be able to obtain a "per-worktree only"
ref store and would need to revert the changes in reflog iteration
API. So let's just wait until then.
add_reflogs_to_pending() is called by reachable.c so by default "git
prune" will examine reflog from all worktrees.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
revision.c | 28 +++-
t/t5304-prune.sh | 16
2 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/revision.c b/revision.c
index 0e98444857..d100b3a3be 100644
--- a/revision.c
+++ b/revision.c
@@ -1132,6 +1132,7 @@ struct all_refs_cb {
int warned_bad_reflog;
struct rev_info *all_revs;
const char *name_for_errormsg;
+ struct ref_store *refs;
};
int ref_excluded(struct string_list *ref_excludes, const char *path)
@@ -1168,6 +1169,7 @@ static void init_all_refs_cb(struct all_refs_cb *cb,
struct rev_info *revs,
cb->all_revs = revs;
cb->all_flags = flags;
revs->rev_input_given = 1;
+ cb->refs = NULL;
}
void clear_ref_exclusion(struct string_list **ref_excludes_p)
@@ -1236,17 +1238,41 @@ static int handle_one_reflog(const char *path, const
struct object_id *oid,
struct all_refs_cb *cb = cb_data;
cb->warned_bad_reflog = 0;
cb->name_for_errormsg = path;
- for_each_reflog_ent(path, handle_one_reflog_ent, cb_data);
+ refs_for_each_reflog_ent(cb->refs, path,
+handle_one_reflog_ent, cb_data);
return 0;
}
+static void add_other_reflogs_to_pending(struct all_refs_cb *cb)
+{
+ struct worktree **worktrees, **p;
+
+ worktrees = get_worktrees(0);
+ for (p = worktrees; *p; p++) {
+ struct worktree *wt = *p;
+
+ if (wt->is_current)
+ continue;
+
+ cb->refs = get_worktree_ref_store(wt);
+ refs_for_each_reflog(cb->refs,
+handle_one_reflog,
+cb);
+ }
+ free_worktrees(worktrees);
+}
+
void add_reflogs_to_pending(struct rev_info *revs, unsigned flags)
{
struct all_refs_cb cb;
cb.all_revs = revs;
cb.all_flags = flags;
+ cb.refs = get_main_ref_store();
for_each_reflog(handle_one_reflog, &cb);
+
+ if (!revs->single_worktree)
+ add_other_reflogs_to_pending(&cb);
}
static void add_cache_tree(struct cache_tree *it, struct rev_info *revs,
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 683bdb031c..6694c19a1e 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -304,4 +304,20 @@ test_expect_success 'prune: handle HEAD in multiple
worktrees' '
test_cmp third-worktree/blob actual
'
+test_expect_success 'prune: handle HEAD reflog in multiple worktrees' '
+ git config core.logAllRefUpdates true &&
+ echo "lost blob for third-worktree" >expected &&
+ (
+ cd third-worktree &&
+ cat ../expected >blob &&
+ git add blob &&
+ git commit -m "second commit in third" &&
+ git reset --hard HEAD^
+ ) &&
+ git prune --expire=now &&
+ SHA1=`git hash-object expected` &&
+ git -C third-worktree show "$SHA1" >actual &&
+ test_cmp expected actual
+'
+
test_done
--
2.11.0.157.gd943d85
[PATCH v4 09/16] refs.c: move for_each_remote_ref_submodule() to submodule.c
Signed-off-by: Nguyễn Thái Ngọc Duy
---
refs.c | 6 --
refs.h | 2 --
submodule.c | 7 +++
3 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/refs.c b/refs.c
index cd61509bc8..7fa19e9309 100644
--- a/refs.c
+++ b/refs.c
@@ -368,12 +368,6 @@ int for_each_remote_ref(each_ref_fn fn, void *cb_data)
return refs_for_each_remote_ref(get_main_ref_store(), fn, cb_data);
}
-int for_each_remote_ref_submodule(const char *submodule, each_ref_fn fn, void
*cb_data)
-{
- return refs_for_each_remote_ref(get_submodule_ref_store(submodule),
- fn, cb_data);
-}
-
int head_ref_namespaced(each_ref_fn fn, void *cb_data)
{
struct strbuf buf = STRBUF_INIT;
diff --git a/refs.h b/refs.h
index a8d6f33703..5d25da227a 100644
--- a/refs.h
+++ b/refs.h
@@ -313,8 +313,6 @@ int for_each_tag_ref_submodule(const char *submodule,
each_ref_fn fn, void *cb_data);
int for_each_branch_ref_submodule(const char *submodule,
each_ref_fn fn, void *cb_data);
-int for_each_remote_ref_submodule(const char *submodule,
- each_ref_fn fn, void *cb_data);
int head_ref_namespaced(each_ref_fn fn, void *cb_data);
int for_each_namespaced_ref(each_ref_fn fn, void *cb_data);
diff --git a/submodule.c b/submodule.c
index e072036e79..98e1f9d3c7 100644
--- a/submodule.c
+++ b/submodule.c
@@ -69,6 +69,13 @@ int is_staging_gitmodules_ok(const struct index_state
*istate)
return 1;
}
+static int for_each_remote_ref_submodule(const char *submodule,
+each_ref_fn fn, void *cb_data)
+{
+ return refs_for_each_remote_ref(get_submodule_ref_store(submodule),
+ fn, cb_data);
+}
+
/*
* Try to update the "path" entry in the "submodule." section of the
* .gitmodules file. Return 0 only if a .gitmodules file was found, a section
--
2.11.0.157.gd943d85
[PATCH v4 11/16] revision.c: --all adds HEAD from all worktrees
Unless single_worktree is set, --all now adds HEAD from all worktrees.
Since reachable.c code does not use setup_revisions(), we need to call
other_head_refs_submodule() explicitly there to have the same effect on
"git prune", so that we won't accidentally delete objects needed by some
other HEADs.
A new FIXME is added because we would need something like
int refs_other_head_refs(struct ref_store *, each_ref_fn, cb_data);
in addition to other_head_refs() to handle it, which might require
int get_submodule_worktrees(const char *submodule, int flags);
It could be a separate topic to reduce the scope of this one.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
reachable.c | 2 ++
revision.c | 14 ++
submodule.c | 2 ++
t/t5304-prune.sh | 12
worktree.c | 22 ++
worktree.h | 8
6 files changed, 60 insertions(+)
diff --git a/reachable.c b/reachable.c
index c62efbfd43..492e87b9fa 100644
--- a/reachable.c
+++ b/reachable.c
@@ -9,6 +9,7 @@
#include "cache-tree.h"
#include "progress.h"
#include "list-objects.h"
+#include "worktree.h"
struct connectivity_progress {
struct progress *progress;
@@ -176,6 +177,7 @@ void mark_reachable_objects(struct rev_info *revs, int
mark_reflog,
/* detached HEAD is not included in the list above */
head_ref(add_one_ref, revs);
+ other_head_refs(add_one_ref, revs);
/* Add all reflog info */
if (mark_reflog)
diff --git a/revision.c b/revision.c
index 8d04516266..0e98444857 100644
--- a/revision.c
+++ b/revision.c
@@ -2133,6 +2133,14 @@ static int handle_revision_pseudo_opt(const char
*submodule,
int argcount;
if (submodule) {
+ /*
+* We need some something like get_submodule_worktrees()
+* before we can go through all worktrees of a submodule,
+* .e.g with adding all HEADs from --all, which is not
+* supported right now, so stick to single worktree.
+*/
+ if (!revs->single_worktree)
+ die("BUG: --single-worktree cannot be used together
with submodule");
refs = get_submodule_ref_store(submodule);
} else
refs = get_main_ref_store();
@@ -2150,6 +2158,12 @@ static int handle_revision_pseudo_opt(const char
*submodule,
if (!strcmp(arg, "--all")) {
handle_refs(refs, revs, *flags, refs_for_each_ref);
handle_refs(refs, revs, *flags, refs_head_ref);
+ if (!revs->single_worktree) {
+ struct all_refs_cb cb;
+
+ init_all_refs_cb(&cb, revs, *flags);
+ other_head_refs(handle_one_ref, &cb);
+ }
clear_ref_exclusion(&revs->ref_excludes);
} else if (!strcmp(arg, "--branches")) {
handle_refs(refs, revs, *flags, refs_for_each_branch_ref);
diff --git a/submodule.c b/submodule.c
index 98e1f9d3c7..61a38adcd4 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1685,6 +1685,8 @@ static int find_first_merges(struct object_array *result,
const char *path,
oid_to_hex(&a->object.oid));
init_revisions(&revs, NULL);
rev_opts.submodule = path;
+ /* FIXME: can't handle linked worktrees in submodules yet */
+ revs.single_worktree = path != NULL;
setup_revisions(ARRAY_SIZE(rev_args)-1, rev_args, &revs, &rev_opts);
/* save all revisions from the above list that contain b */
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index cba45c7be9..683bdb031c 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -292,4 +292,16 @@ test_expect_success 'prune: handle index in multiple
worktrees' '
test_cmp second-worktree/blob actual
'
+test_expect_success 'prune: handle HEAD in multiple worktrees' '
+ git worktree add --detach third-worktree &&
+ echo "new blob for third-worktree" >third-worktree/blob &&
+ git -C third-worktree add blob &&
+ git -C third-worktree commit -m "third" &&
+ rm .git/worktrees/third-worktree/index &&
+ test_must_fail git -C third-worktree show :blob &&
+ git prune --expire=now &&
+ git -C third-worktree show HEAD:blob >actual &&
+ test_cmp third-worktree/blob actual
+'
+
test_done
diff --git a/worktree.c b/worktree.c
index e28ffbeb09..389e2e952c 100644
--- a/worktree.c
+++ b/worktree.c
@@ -386,3 +386,25 @@ int submodule_uses_worktrees(const char *path)
closedir(dir);
return ret;
}
+
+int other_head_refs(each_ref_fn fn, void *cb_data)
+{
+ struct worktree **worktrees, **p;
+ int ret = 0;
+
+ worktrees = get_worktrees(0);
+ for (p = worktrees; *p; p++) {
+ struct worktree *wt = *p;
+ struct ref_store *refs;
+
+ if (wt->is_current)
+ continue;
+
+ refs
[PATCH v4 15/16] refs.c: remove fallback-to-main-store code get_submodule_ref_store()
At this state, there are three get_submodule_ref_store() callers:
- for_each_remote_ref_submodule()
- handle_revision_pseudo_opt()
- resolve_gitlink_ref()
The first two deal explicitly with submodules (and we should never fall
back to the main ref store as a result). They are only called from
submodule.c:
- find_first_merges()
- submodule_needs_pushing()
- push_submodule()
The last one, as its name implies, deals only with submodules too, and
the "submodule" (path) argument must be a non-NULL, non-empty string.
So, this "if NULL or empty string" code block should never ever
trigger. And it's wrong to fall back to the main ref store
anyway. Delete it.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
refs.c | 11 +++
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/refs.c b/refs.c
index 8c989ffec7..a0c5078901 100644
--- a/refs.c
+++ b/refs.c
@@ -1587,6 +1587,9 @@ struct ref_store *get_submodule_ref_store(const char
*submodule)
char *to_free = NULL;
size_t len;
+ if (!submodule)
+ return NULL;
+
if (submodule) {
len = strlen(submodule);
while (len && is_dir_sep(submodule[len - 1]))
@@ -1595,14 +1598,6 @@ struct ref_store *get_submodule_ref_store(const char
*submodule)
return NULL;
}
- if (!submodule || !*submodule) {
- /*
-* FIXME: This case is ideally not allowed. But that
-* can't happen until we clean up all the callers.
-*/
- return get_main_ref_store();
- }
-
if (submodule[len])
/* We need to strip off one or more trailing slashes */
submodule = to_free = xmemdupz(submodule, len);
--
2.11.0.157.gd943d85
[PATCH v4 14/16] rev-list: expose and document --single-worktree
Signed-off-by: Nguyễn Thái Ngọc Duy
---
Documentation/rev-list-options.txt | 8
revision.c | 2 ++
2 files changed, 10 insertions(+)
diff --git a/Documentation/rev-list-options.txt
b/Documentation/rev-list-options.txt
index a6cf9eb380..7d860bfca1 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -184,6 +184,14 @@ explicitly.
Pretend as if all objects mentioned by reflogs are listed on the
command line as ``.
+--single-worktree::
+ By default, all working trees will be examined by the
+ following options when there are more than one (see
+ linkgit:git-worktree[1]): `--all`, `--reflog` and
+ `--indexed-objects`.
+ This option forces them to examine the current working tree
+ only.
+
--ignore-missing::
Upon seeing an invalid object name in the input, pretend as if
the bad input was not given.
diff --git a/revision.c b/revision.c
index d100b3a3be..6eba4131b4 100644
--- a/revision.c
+++ b/revision.c
@@ -2251,6 +2251,8 @@ static int handle_revision_pseudo_opt(const char
*submodule,
return error("invalid argument to --no-walk");
} else if (!strcmp(arg, "--do-walk")) {
revs->no_walk = 0;
+ } else if (!strcmp(arg, "--single-worktree")) {
+ revs->single_worktree = 1;
} else {
return 0;
}
--
2.11.0.157.gd943d85
[PATCH v4 07/16] refs: add refs_head_ref()
Signed-off-by: Nguyễn Thái Ngọc Duy
---
refs.c | 23 +--
refs.h | 2 ++
2 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/refs.c b/refs.c
index ea8e6b9f42..b3a0a24469 100644
--- a/refs.c
+++ b/refs.c
@@ -1248,27 +1248,30 @@ int refs_rename_ref_available(struct ref_store *refs,
return ok;
}
-int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
+int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
{
struct object_id oid;
int flag;
- if (submodule) {
- if (resolve_gitlink_ref(submodule, "HEAD", oid.hash) == 0)
- return fn("HEAD", &oid, 0, cb_data);
-
- return 0;
- }
-
- if (!read_ref_full("HEAD", RESOLVE_REF_READING, oid.hash, &flag))
+ if (!refs_read_ref_full(refs, "HEAD", RESOLVE_REF_READING,
+ oid.hash, &flag))
return fn("HEAD", &oid, flag, cb_data);
return 0;
}
+int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
+{
+ struct ref_store *refs = get_submodule_ref_store(submodule);
+
+ if (!refs)
+ return -1;
+ return refs_head_ref(refs, fn, cb_data);
+}
+
int head_ref(each_ref_fn fn, void *cb_data)
{
- return head_ref_submodule(NULL, fn, cb_data);
+ return refs_head_ref(get_main_ref_store(), fn, cb_data);
}
struct ref_iterator *refs_ref_iterator_begin(
diff --git a/refs.h b/refs.h
index 6daa78eb50..8073f8ab56 100644
--- a/refs.h
+++ b/refs.h
@@ -275,6 +275,8 @@ typedef int each_ref_fn(const char *refname,
* modifies the reference also returns a nonzero value to immediately
* stop the iteration. Returned references are sorted.
*/
+int refs_head_ref(struct ref_store *refs,
+ each_ref_fn fn, void *cb_data);
int refs_for_each_ref(struct ref_store *refs,
each_ref_fn fn, void *cb_data);
int refs_for_each_ref_in(struct ref_store *refs, const char *prefix,
--
2.11.0.157.gd943d85
[PATCH v4 10/16] refs: remove dead for_each_*_submodule()
These are used in revision.c. After the last patch they are replaced
with the refs_ version. Delete them.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
Documentation/technical/api-ref-iteration.txt | 7 ++
refs.c| 33 ---
refs.h| 10
3 files changed, 2 insertions(+), 48 deletions(-)
diff --git a/Documentation/technical/api-ref-iteration.txt
b/Documentation/technical/api-ref-iteration.txt
index 37379d8337..46c3d5c355 100644
--- a/Documentation/technical/api-ref-iteration.txt
+++ b/Documentation/technical/api-ref-iteration.txt
@@ -32,11 +32,8 @@ Iteration functions
* `for_each_glob_ref_in()` the previous and `for_each_ref_in()` combined.
-* `head_ref_submodule()`, `for_each_ref_submodule()`,
- `for_each_ref_in_submodule()`, `for_each_tag_ref_submodule()`,
- `for_each_branch_ref_submodule()`, `for_each_remote_ref_submodule()`
- do the same as the functions described above but for a specified
- submodule.
+* Use `refs_` API for accessing submodules. The submodule ref store could
+ be obtained with `get_submodule_ref_store()`.
* `for_each_rawref()` can be used to learn about broken ref and symref.
diff --git a/refs.c b/refs.c
index 7fa19e9309..8c989ffec7 100644
--- a/refs.c
+++ b/refs.c
@@ -336,12 +336,6 @@ int for_each_tag_ref(each_ref_fn fn, void *cb_data)
return refs_for_each_tag_ref(get_main_ref_store(), fn, cb_data);
}
-int for_each_tag_ref_submodule(const char *submodule, each_ref_fn fn, void
*cb_data)
-{
- return refs_for_each_tag_ref(get_submodule_ref_store(submodule),
-fn, cb_data);
-}
-
int refs_for_each_branch_ref(struct ref_store *refs, each_ref_fn fn, void
*cb_data)
{
return refs_for_each_ref_in(refs, "refs/heads/", fn, cb_data);
@@ -352,12 +346,6 @@ int for_each_branch_ref(each_ref_fn fn, void *cb_data)
return refs_for_each_branch_ref(get_main_ref_store(), fn, cb_data);
}
-int for_each_branch_ref_submodule(const char *submodule, each_ref_fn fn, void
*cb_data)
-{
- return refs_for_each_branch_ref(get_submodule_ref_store(submodule),
- fn, cb_data);
-}
-
int refs_for_each_remote_ref(struct ref_store *refs, each_ref_fn fn, void
*cb_data)
{
return refs_for_each_ref_in(refs, "refs/remotes/", fn, cb_data);
@@ -1254,15 +1242,6 @@ int refs_head_ref(struct ref_store *refs, each_ref_fn
fn, void *cb_data)
return 0;
}
-int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
-{
- struct ref_store *refs = get_submodule_ref_store(submodule);
-
- if (!refs)
- return -1;
- return refs_head_ref(refs, fn, cb_data);
-}
-
int head_ref(each_ref_fn fn, void *cb_data)
{
return refs_head_ref(get_main_ref_store(), fn, cb_data);
@@ -1323,11 +1302,6 @@ int for_each_ref(each_ref_fn fn, void *cb_data)
return refs_for_each_ref(get_main_ref_store(), fn, cb_data);
}
-int for_each_ref_submodule(const char *submodule, each_ref_fn fn, void
*cb_data)
-{
- return refs_for_each_ref(get_submodule_ref_store(submodule), fn,
cb_data);
-}
-
int refs_for_each_ref_in(struct ref_store *refs, const char *prefix,
each_ref_fn fn, void *cb_data)
{
@@ -1349,13 +1323,6 @@ int for_each_fullref_in(const char *prefix, each_ref_fn
fn, void *cb_data, unsig
prefix, fn, 0, flag, cb_data);
}
-int for_each_ref_in_submodule(const char *submodule, const char *prefix,
- each_ref_fn fn, void *cb_data)
-{
- return refs_for_each_ref_in(get_submodule_ref_store(submodule),
- prefix, fn, cb_data);
-}
-
int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix,
each_ref_fn fn, void *cb_data,
unsigned int broken)
diff --git a/refs.h b/refs.h
index 5d25da227a..78a26400b6 100644
--- a/refs.h
+++ b/refs.h
@@ -304,16 +304,6 @@ int for_each_glob_ref(each_ref_fn fn, const char *pattern,
void *cb_data);
int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,
const char *prefix, void *cb_data);
-int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);
-int for_each_ref_submodule(const char *submodule,
- each_ref_fn fn, void *cb_data);
-int for_each_ref_in_submodule(const char *submodule, const char *prefix,
- each_ref_fn fn, void *cb_data);
-int for_each_tag_ref_submodule(const char *submodule,
- each_ref_fn fn, void *cb_data);
-int for_each_branch_ref_submodule(const char *submodule,
- each_ref_fn fn, void *cb_data);
-
int head_ref_namespaced(each_ref_fn fn, void *cb_data);
int for_each_namespaced_ref(each_ref_fn fn, void *cb_data);
--
2.11.0.157.gd943d85
[PATCH v4 16/16] refs.c: reindent get_submodule_ref_store()
With the new "if (!submodule) return NULL;" code added in the previous
commit, we don't need to check if submodule is not NULL anymore.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
refs.c | 12 +---
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/refs.c b/refs.c
index a0c5078901..206af61d62 100644
--- a/refs.c
+++ b/refs.c
@@ -1590,13 +1590,11 @@ struct ref_store *get_submodule_ref_store(const char
*submodule)
if (!submodule)
return NULL;
- if (submodule) {
- len = strlen(submodule);
- while (len && is_dir_sep(submodule[len - 1]))
- len--;
- if (!len)
- return NULL;
- }
+ len = strlen(submodule);
+ while (len && is_dir_sep(submodule[len - 1]))
+ len--;
+ if (!len)
+ return NULL;
if (submodule[len])
/* We need to strip off one or more trailing slashes */
--
2.11.0.157.gd943d85
Re: splitting off shell test framework
I got a helpful response to the following question almost 5 years ago: On 12 November 2012 at 23:09, Adam Spiers wrote: > On Mon, Nov 12, 2012 at 6:18 PM, Drew Northup wrote: >> On Mon, Nov 12, 2012 at 11:37 AM, Adam Spiers wrote: >>> As it turned out to be fairly easy, I was wondering if there would be >>> any interest in doing this more formally, i.e. splitting off the >>> framework so that it could be used and improved outside the scope of >>> git development? Of course this would pose the question how git would >>> consume this new project without any risk of destabilisation. I'm >>> guessing that simply using a git submodule would solve the problem, >>> but ICBW ... >>> >>> Just an idea. Interesting, or terrible? :) >> >> Done at least once already: >> >> http://comments.gmane.org/gmane.comp.version-control.git/201591 > > Nice! So hopefully someone will submit patches to build a two-way bridge > via git subtree. Having them diverge would be sad. but sadly since then gmane has shuffled off its mortal coil and I can't remember / find what this URL referred to. Please could someone point me at a working link? Many thanks! Adam
Re: [BUG] rebase -i with only empty commits
Hi, On Wed, 23 Aug 2017, Stephan Beyer wrote: > On 08/23/2017 01:08 AM, Stefan Beller wrote: > > The editor opened proposing the following instruction sheet, > > which in my opinion is buggy: > > > > pick 1234 some commit > > exec make > > pick 2345 another commit > > exec make > > pick 3456 third commit > > # pick 4567 empty commit > > exec make > > pick 5678 yet another commit > > exec make > > This reminds me of another bug I stumbled over recently regarding empty > commits. > > Do this: > # repo preparation: > git init > :> file1 > git add file1 > git commit -m "add file1" > :> file2 > git add file2 > git commit -m "add file2" > > # the bug: > git checkout -b to-be-rebased master^ > git commit --allow-empty -m "empty commit" > git rebase -i master > > It says "Nothing to do". > Unsurprisingly, the problem persists when you apply other empty commits: > > git commit --allow-empty -m "another empty commit" > git rebase -i master > > Adding a "real" commit solves the problem: > > :>file3 > git add file3 > git commit -m "add file3" > > Adding further empty commits is no problem: > > git commit --allow-empty -m "yet another empty commit" > > So the problem seems to be that rebase -i (like rebase without -i) > considers "empty commits" as commits to be ignored. However, when using > rebase -i one expects that you can include the empty commit... > > Also, the behavior is odd. When I only have empty commits, a "git rebase > master" works as expected like a "git reset --hard master" but "git > rebase -i" does nothing. > > The expected behavior would be that the editor shows up with a > git-rebase-todo like: > # pick 3d0f6c49 empty commit > # pick bbbc5941 another empty commit > noop These days, I reflexively type `rebase -ki` instead of `rebase -i`. Maybe you want to do that, too? Ciao, Dscho
Re: [BUG] rebase -i with only empty commits
Hi, On 08/23/2017 04:40 PM, Johannes Schindelin wrote: > These days, I reflexively type `rebase -ki` instead of `rebase -i`. Maybe > you want to do that, too? That's a very valuable hint, thank you very much! Best Stephan
Re: Please fix the useless email prompts
On Sun, Aug 20, 2017 at 04:56:50PM -0700, Junio C Hamano wrote: > Andrew Ardill writes: > > > Is there any reason `git pull` can't delay that check until the point > > where it actually tries to create a new commit? It's fair enough to > > error if a new commit needs to be made, and there is no user > > configured, but for the use cases discussed here it seems a little > > eager to error on the chance that the user will be needed. > > I personally do not think it is a good trade-off. > > In theory [*1*], > [...] > *1* We actually might already do such an "optimization"; I didn't > check. I think we already do. Reflogs do not ask for strict identity, and we've addressed similar cases (e.g., 1e461c4f1fc which was motivated by pull.rebase not handling this). > But before running "fetch", you cannot tell if the "pull" will > fast-forward, so such an "optimization" may actually be a net loss > for end users, who have to wait for network delay only to be told > that you'd end up with a history with bogus identity and need to > redo the operation after fixing your identity. Then after that, > they are likely to do another "git pull", which will avoid the cost > of retransmission of objects if (and only if) the initial "git pull" > uses remote-tracking branches. I agree that in the common case where the command might or might not need to create a commit, it's nicer if we tell the user up front. But there are also cases where the user can reasonably expect that a commit will not need to be created. Certainly --ff-only is one hint. But in general a pull-only repository for testing will just see repeated fetch+fast-forward pulls. People with that kind of setup would see it as a regression if pull started failing to say "I don't know yet whether we'll need to create a commit, but I'm failing early to let you know that it won't work". If we could reliably tell the difference between those two cases, it might be worth doing the up-front check. But I'm not sure we can do that without declaring that people in the ff-only case should be using a different workflow (e.g., fetch + "reset --hard"). -Peff
Re: [PATCH 0/2] http: handle curl with vendor backports
On Sun, Aug 20, 2017 at 09:28:20AM -0700, Junio C Hamano wrote: > > Yes, I agree that these are an improvement regardless. If we follow > > through on the cut-off to 7.19.4, then the CURLPROTO ones all go away. > > But I don't mind rebasing any cut-off proposal on top of this work. > > Yeah I came to a similar conclusion and was about asking if you feel > the same way that your series should be made on top of Tom's fixes. > > The aspect of that series I do like the most is to base our > decisions on features, not versions, and I also wonder if we can do > similar in your "abandon too old ones" series, too. Yeah, I don't mind moving to feature flags where we can (though some features do not have a useful flag; e.g., the only way to know whether we must be strdup curl_easy_setopt() arguments is by checking the curl version). One annoying thing about "feature" flags instead of version flags is that it takes a lot of legwork to figure out how old those features are (whereas with the versions I was able to look that up in the curl history pretty easily). Since people adding the feature flag generally do that legwork, it's probably worth having a comment for each mentioning the general vintage (or maybe the commit message is an OK place for that). I actually wonder if it is worth defining our own readable flags in a big table at the beginning of the file, like: /* * introduced in curl 7.19.4, but backported by some distros like * RHEL. We can identify it by the presence of the PROTO flags. */ #ifdef CURLPROTO_HTTP #define CURL_SUPPORTS_PROTOCOL_REDIRECTION #endif That keeps the logic in one place (where it can be changed if we later find that the define we picked for our feature isn't quite accurate). And then the #ifdefs sprinkled through the code itself become self-documenting. -Peff
Re: splitting off shell test framework
On Wed, Aug 23, 2017 at 02:46:30PM +0100, Adam Spiers wrote: > >> Done at least once already: > >> > >> http://comments.gmane.org/gmane.comp.version-control.git/201591 > [...] > > but sadly since then gmane has shuffled off its mortal coil and I can't > remember / find what this URL referred to. Please could someone > point me at a working link? Try: https://public-inbox.org/git/?q=gmane:201591 Public-inbox uses message-ids as its primary key, but keeps a static mapping of gmane articles to message-ids. But the corpus of gmane ids isn't growing anymore, and public-inbox URLs can be trivially converted to other systems which index on the message id[1]. -Peff [1] I actually keep a local archive and convert public-inbox URLs into local requests that I view in mutt.
Re: [PATCH/RFC] push: anonymize URL in error output
On Wed, Aug 23, 2017 at 12:49:29PM +0300, Ivan Vyshnevskyi wrote:
> Commits 47abd85 (fetch: Strip usernames from url's before storing them,
> 2009-04-17) and later 882d49c (push: anonymize URL in status output,
> 2016-07-14) made fetch and push strip the authentication part of the
> remote URLs when used in the merge-commit messages or status outputs.
> The URLs that are part of the error messages were not anonymized.
>
> A commonly used pattern for storing artifacts from a build server in a
> remote repository utilizes a "secure" environment variable with
> credentials to embed them in the URL and execute a push. Given enough
> runs, an intermittent network failure will cause a push to fail, leaving
> a non-anonymized URL in the build log.
>
> To prevent that, reuse the same anonymizing function to scrub
> credentials from URL in the push error output.
This makes sense. I suspect that most errors we output should be using
the anonymized URL. Did you poke around for other calls?
The general structure of the patch looks good, but I have a few minor
comments below.
> Not sure how much of the background should be included in the commit message.
> The "commonly used pattern" I mention could be found in the myriad of
> online tutorials and looks something like this:
My knee-jerk reaction is if it's worth writing after the dashes, it's
worth putting in the commit message.
However, in the case I think it is OK as-is (the motivation of "we
already avoid leaking auth info to stdout, so we should do the same for
error messages" seems self-contained and reasonable).
> diff --git a/builtin/push.c b/builtin/push.c
> index 03846e837..59f3bc975 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -336,7 +336,7 @@ static int push_with_options(struct transport *transport,
> int flags)
> err = transport_push(transport, refspec_nr, refspec, flags,
>&reject_reasons);
> if (err != 0)
> - error(_("failed to push some refs to '%s'"), transport->url);
> + error(_("failed to push some refs to '%s'"),
> transport_anonymize_url(transport->url));
This leaks the return value. That's probably not a _huge_ deal since the
program is likely to exit, but it's a bad pattern. I wonder if we should
be setting up transport->anonymous_url preemptively, and just let its
memory belong to the transport struct.
> diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
> index d38bf3247..0b6fb6252 100755
> --- a/t/t5541-http-push-smart.sh
> +++ b/t/t5541-http-push-smart.sh
> @@ -377,5 +377,23 @@ test_expect_success 'push status output scrubs password'
> '
> grep "^To $HTTPD_URL/smart/test_repo.git" status
> '
>
> +cat >"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update" < +#!/bin/sh
> +exit 1
> +EOF
> +chmod a+x "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update"
> +
> +cat >exp < +error: failed to push some refs to '$HTTPD_URL/smart/test_repo.git'
> +EOF
I know the t5541 script, which is old and messy, led you into these bad
constructs. But usually in modern tests we:
1. Try to keep all commands inside test_expect blocks to catch
unexpected failures or unwanted output.
2. Use write_script for writing scripts, like:
write_script "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update"
<<-\EOF
exit 1
EOF
3. Backslash our here-doc delimiter to suppress interpolation.
> +test_expect_success 'failed push status output scrubs password' '
> + cd "$ROOT_PATH"/test_repo_clone &&
> + test_must_fail git push "$HTTPD_URL_USER_PASS/smart/test_repo.git"
> +HEAD:scrub_err 2>stderr &&
> + grep "^error: failed to push some refs" stderr >act &&
> + test_i18ncmp exp act
> +'
> +rm -f "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update"
Similarly, this "rm" should probably be a test_when_finished in the
block with the write_script (unless you really need to carry it over
several test_expect blocks, in which case there should be an explicit
test_expect cleaning it up).
Instead of grepping for the exact error, should we instead grep for the
password to make sure it is not present on _any_ line?
-Peff
Re: sequencer status
Hi, On Wed, Aug 23, 2017 at 10:10 AM, Nicolas Morey-Chaisemartin wrote: > Hi, > > I've created a small tool to display the current sequencer status. > It mimics what Magit does to display what was done and what is left to do. > > As someone who often rebase large series of patches (also works with am, > revert, cherry-pick), I really needed the feature and use this daily. Yeah, many people use the interactive rebase a lot so I think it could be very interesting. > It's available here: > https://github.com/nmorey/git-sequencer-status > SUSE and Fedora packages are available here: > https://build.opensuse.org/package/show/home:NMoreyChaisemartin:git-tools/git-sequencer-status > > It's not necessary very robust yet. Most of the time I use simple rebase so > there are a lot of untested corner cases. > > Here is an example output: > $ sequencer-status > # Interactive rebase: master onto rebase_conflict > exectrue > picke54d993 Fourth commit > exectrue > *pick b064629 Third commit > exectrue > pick174c933 Second commit > onto773cb23 Alternate second It is displaying the steps that have already been performed, right? I wonder if people might want more about the current step (but maybe that belongs to `git status`) or perhaps the not yet performed states (and maybe even a way to edit the todo list?) > Two questions: > - Could this be a candidate for contrib/ ? It seems to me that these days we don't often add new tools to contrib/. > - Would it be interesting to add the relevant code to sequencer.c so that all > sequencer based commands could have a --status option ? Yeah, it's probably better if it's integrated in git, either as a --status option in some commands, or perhaps as an option of `git status`. Thanks, Christian.
Re: How to force a push to succeed?
Hi Jeffrey, Jeffrey Walton wrote: > From another testing machine, it looks like the changes have not been > backed out. The previous operation un-did the ADX gear because it was > an evolutionary dead-end. > > via$ git pull > From https://github.com/noloader/cryptopp > + 66654dd...559fc3b master -> origin/master (forced update) The Git User Manual[1] explains: | We have already seen how to keep remote-tracking branches up to date | with git-fetch(1), and how to merge two branches. So you can merge | in changes from the original repository’s master branch with: | | $ git fetch | $ git merge origin/master | | However, the git-pull(1) command provides a way to do this in one step: | | $ git pull origin master There is also some discussion of that in "git help pull". But that doesn't appear to be what you want to do. You are not looking to incorporate remote changes into your local branch, preserving any changes that are in your local branch --- instead, you are looking to move to what the remote server has, overwriting any changes that are in your local branch. You can do that with git fetch git reset --hard origin/master See also the discussion of --force in "git help push". I also recommend looking at the Git Tutorial[2]. When it introduces "git reset --hard" to back out changes, it says: | $ git reset --hard HEAD^ # reset your current branch and working | # directory to its state at HEAD^ | | Be careful with that last command: in addition to losing any changes | in the working directory, it will also remove all later commits from | this branch. If this branch is the only branch containing those | commits, they will be lost. Also, don’t use git reset on a | publicly-visible branch that other developers pull from, as it will | force needless merges on other developers to clean up the history. | If you need to undo changes that you have pushed, use git revert | instead. If you have ideas about how it could explain this better (or even better, how commands could behave to avoid needing such explanation) then I would be happy to hear them. (Side note: I am starting to wonder whether what you were looking for was something like "git revert", especially if this is a project involving more than one person.) > You know, I look at how fucked up yet another simple workflow is, and > all I can do is wonder. It is absolutely amazing. Its like the project > goes out of its way to make simple tasks difficult. I understand your frustration, but accusing people of trying to make your life difficult isn't a particularly productive way to get help from them. For the future, some ways to get good results are - to describe the background of what you are trying to do - to describe where you looked for answers, so that we know what documentation to improve The #git IRC channel on freenode can also be helpful for real-time help. But I encourage you to also keep writing here so that we know what documentation and workflows to improve. Thanks and hope that helps, Jonatha [1] https://www.kernel.org/pub/software/scm/git/docs/user-manual.html#getting-updates-With-git-pull [2] https://www.kernel.org/pub/software/scm/git/docs/gittutorial.html
Re: How to force a push to succeed?
Am 23.08.2017 um 00:26 schrieb Jeffrey Walton: You know, I look at how fucked up yet another simple workflow is, and all I can do is wonder. It is absolutely amazing. Its like the project goes out of its way to make simple tasks difficult. No, no. So simple your task might look, in the end you have LOST DATA! It is a design goal of Git to make it difficult to lose data. -- Hannes
Re
Aufmerksamkeit, Bewerben Sie sich für eine schnelle und bequeme Darlehen zu bezahlen Rechnungen, persönliche Darlehen, Studenten Darlehen, Weihnachtsdarlehen, Hypothekendarlehen, Konsolidierungsdarlehen und ein Neugeschäft zu starten oder Ihre Projekte mit einem günstigsten Zinssatz von 3% zu refinanzieren. Kontaktieren Sie uns heute über: (tomcrist...@yahoo.com) mit Darlehensbetrag benötigt.Wir sind zertifiziert, registriert und legitime Kreditgeber. Sie können uns heute kontaktieren, wenn Sie daran interessiert sind, dieses Darlehen zu erhalten, kontaktieren Sie uns für weitere Informationen über den Kredit-Prozess, Prozess wie die Darlehensbedingungen und wie das Darlehen an Sie übertragen werden. Wir brauchen Ihre dringende Antwort, wenn Sie interessiert sind. HINWEIS: Alle Antworten sollten an folgende Adresse weitergeleitet werden: (tomcrist...@yahoo.com) Danke. C.E.O / Tom Crist.
Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf
Martin Ågren writes:
> strbuf_setlen(., 0) writes '\0' to sb.buf[0], where buf is either
> allocated and unique to sb, or the global slopbuf. The slopbuf is meant
> to provide a guarantee that buf is not NULL and that a freshly
> initialized buffer contains the empty string, but it is not supposed to
> be written to. That strbuf_setlen writes to slopbuf has at least two
> implications:
>
> First, it's wrong in principle. Second, it might be hiding misuses which
> are just waiting to wreak havoc. Third, ThreadSanitizer detects a race
> when multiple threads write to slopbuf at roughly the same time, thus
> potentially making any more critical races harder to spot.
There are two hard things in computer science ;-).
> Suggested-by: Junio C Hamano
> Signed-off-by: Martin Ågren
> ---
> v2: no "ifdef TSAN"; moved check from strbuf_reset into strbuf_setlen
Looks much better. I have a mild objection to "suggested-by",
though. It makes it sound as if this were my itch, but it is not.
All the credit for being motivate to fix the issue should go to you.
For what I did during the review of the previous one to lead to this
simpler version, if you want to document it, "helped-by" would be
more appropriate.
> strbuf.h | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/strbuf.h b/strbuf.h
> index e705b94db..7496cb8ec 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -147,7 +147,10 @@ static inline void strbuf_setlen(struct strbuf *sb,
> size_t len)
> if (len > (sb->alloc ? sb->alloc - 1 : 0))
> die("BUG: strbuf_setlen() beyond buffer");
> sb->len = len;
> - sb->buf[len] = '\0';
> + if (sb->buf != strbuf_slopbuf)
> + sb->buf[len] = '\0';
> + else
> + assert(!strbuf_slopbuf[0]);
> }
>
> /**
Re: [BUG] rebase -i with only empty commits
On Wed, Aug 23, 2017 at 8:19 AM, Stephan Beyer wrote: > Hi, > > On 08/23/2017 04:40 PM, Johannes Schindelin wrote: >> These days, I reflexively type `rebase -ki` instead of `rebase -i`. Maybe >> you want to do that, too? > > That's a very valuable hint, thank you very much! While -k side steps the original problem, it seems like it would have helped me, too. Is there any value in discussing turning it on by default?
Re: Please fix the useless email prompts
Jeff King writes: > If we could reliably tell the difference between those two cases, it > might be worth doing the up-front check. But I'm not sure we can do that > without declaring that people in the ff-only case should be using a > different workflow (e.g., fetch + "reset --hard"). Yup, it _might_ be nicer but I tend to think probably it is not worth the potential trouble. Thanks.
Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf
On 23 August 2017 at 19:24, Junio C Hamano wrote: > Martin Ågren writes: > >> strbuf_setlen(., 0) writes '\0' to sb.buf[0], where buf is either >> allocated and unique to sb, or the global slopbuf. The slopbuf is meant >> to provide a guarantee that buf is not NULL and that a freshly >> initialized buffer contains the empty string, but it is not supposed to >> be written to. That strbuf_setlen writes to slopbuf has at least two >> implications: >> >> First, it's wrong in principle. Second, it might be hiding misuses which >> are just waiting to wreak havoc. Third, ThreadSanitizer detects a race >> when multiple threads write to slopbuf at roughly the same time, thus >> potentially making any more critical races harder to spot. > > There are two hard things in computer science ;-). Indeed. :-) >> Suggested-by: Junio C Hamano >> Signed-off-by: Martin Ågren >> --- >> v2: no "ifdef TSAN"; moved check from strbuf_reset into strbuf_setlen > > Looks much better. I have a mild objection to "suggested-by", > though. It makes it sound as if this were my itch, but it is not. > > All the credit for being motivate to fix the issue should go to you. > For what I did during the review of the previous one to lead to this > simpler version, if you want to document it, "helped-by" would be > more appropriate. Ok, so that's two things to tweak in the commit message. I'll hold off on v3 in case I get some more feedback the coming days. Thanks. Martin
[PATCH 1/2] Documentation/user-manual: update outdated example output
Since commit f7673490 ("more terse push output", 2007-11-05), git push
has a completely different output format than the one shown in the user
manual for a non-fast-forward push.
Signed-off-by: Martin Ågren
---
I'd say it's "not very many read this and immediately tried it out" and
not "nobody read this for the last ten years".
Documentation/user-manual.txt | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt
index bc2929867..d3c53b513 100644
--- a/Documentation/user-manual.txt
+++ b/Documentation/user-manual.txt
@@ -2044,10 +2044,12 @@ If a push would not result in a
<> of the
remote branch, then it will fail with an error like:
-
-error: remote 'refs/heads/master' is not an ancestor of
- local 'refs/heads/master'.
- Maybe you are not up-to-date and need to pull first?
-error: failed to push to 'ssh://yourserver.com/~you/proj.git'
+ ! [rejected]master -> master (non-fast-forward)
+error: failed to push some refs to '...'
+hint: Updates were rejected because the tip of your current branch is behind
+hint: its remote counterpart. Integrate the remote changes (e.g.
+hint: 'git pull ...') before pushing again.
+hint: See the 'Note about fast-forwards' in 'git push --help' for details.
-
This can happen, for example, if you:
--
2.14.1.151.g45c1275a3.dirty
[PATCH 2/2] treewide: correct several "up-to-date" to "up to date"
Follow the Oxford style, which says to use "up-to-date" before the noun, but "up to date" after it. Don't change plumbing (specifically send-pack.c, but transport.c (git push) also has the same string). This was produced by grepping for "up-to-date" and "up to date". It turned out we only had to edit in one direction, removing the hyphens. Fix a typo in Documentation/git-diff-index.txt while we're there. Reported-by: Jeffrey Manian Reported-by: STEVEN WHITE Signed-off-by: Martin Ågren --- I figure "Already up-to-date." is "[X is|I am] already up to date." as opposed to "Already [have] up-to-date [X].". Documentation/git-apply.txt | 4 ++-- Documentation/git-cvsserver.txt | 2 +- Documentation/git-diff-index.txt | 6 +++--- Documentation/git-merge.txt | 2 +- Documentation/git-rebase.txt | 2 +- Documentation/git-rerere.txt | 2 +- Documentation/git-rm.txt | 2 +- Documentation/git-svn.txt | 2 +- Documentation/git-update-index.txt| 2 +- Documentation/gitcore-tutorial.txt| 8 Documentation/githooks.txt| 2 +- Documentation/gitrepository-layout.txt| 2 +- Documentation/gittutorial.txt | 2 +- Documentation/merge-options.txt | 2 +- Documentation/technical/pack-protocol.txt | 2 +- Documentation/technical/trivial-merge.txt | 4 ++-- Documentation/user-manual.txt | 2 +- t/t6040-tracking-info.sh | 4 ++-- contrib/examples/git-merge.sh | 4 ++-- contrib/examples/git-resolve.sh | 2 +- contrib/subtree/t/t7900-subtree.sh| 2 +- git-merge-octopus.sh | 2 +- builtin/merge.c | 4 ++-- merge-recursive.c | 2 +- notes-merge.c | 2 +- remote.c | 2 +- unpack-trees.c| 2 +- git-gui/po/README | 2 +- git-p4.py | 2 +- templates/hooks--pre-rebase.sample| 2 +- 30 files changed, 40 insertions(+), 40 deletions(-) diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt index 631cbd840..4ebc3d327 100644 --- a/Documentation/git-apply.txt +++ b/Documentation/git-apply.txt @@ -66,7 +66,7 @@ OPTIONS disables it is in effect), make sure the patch is applicable to what the current index file records. If the file to be patched in the working tree is not - up-to-date, it is flagged as an error. This flag also + up to date, it is flagged as an error. This flag also causes the index file to be updated. --cached:: @@ -259,7 +259,7 @@ treats these changes as follows. If `--index` is specified (explicitly or implicitly), then the submodule commits must match the index exactly for the patch to apply. If any of the submodules are checked-out, then these check-outs are completely -ignored, i.e., they are not required to be up-to-date or clean and they +ignored, i.e., they are not required to be up to date or clean and they are not updated. If `--index` is not specified, then the submodule commits in the patch diff --git a/Documentation/git-cvsserver.txt b/Documentation/git-cvsserver.txt index a336ae5f6..ba90066f1 100644 --- a/Documentation/git-cvsserver.txt +++ b/Documentation/git-cvsserver.txt @@ -223,7 +223,7 @@ access method and requested operation. That means that even if you offer only read access (e.g. by using the pserver method), 'git-cvsserver' should have write access to the database to work reliably (otherwise you need to make sure -that the database is up-to-date any time 'git-cvsserver' is executed). +that the database is up to date any time 'git-cvsserver' is executed). By default it uses SQLite databases in the Git directory, named `gitcvs..sqlite`. Note that the SQLite backend creates diff --git a/Documentation/git-diff-index.txt b/Documentation/git-diff-index.txt index a17150695..b38067771 100644 --- a/Documentation/git-diff-index.txt +++ b/Documentation/git-diff-index.txt @@ -85,7 +85,7 @@ a 'git write-tree' + 'git diff-tree'. Thus that's the default mode. The non-cached version asks the question: show me the differences between HEAD and the currently checked out - tree - index contents _and_ files that aren't up-to-date + tree - index contents _and_ files that aren't up to date which is obviously a very useful question too, since that tells you what you *could* commit. Again, the output matches the 'git diff-tree -r' @@ -100,8 +100,8 @@ have not actually done a 'git update-index' on it yet - there is no torvalds@ppc970:~/v2.6/linux> git diff-index --abbrev HEAD :100644 100664 7476bb... 00... kernel/sched.c -i.e., it shows that the tree has changed, and that `kernel/sched.c` has is -not up-to-date and may contain new stuff. The all-zero sha1 means that to +i
Re: sequencer status
Nicolas Morey-Chaisemartin writes: > Two questions: > - Could this be a candidate for contrib/ ? > - Would it be interesting to add the relevant code to sequencer.c > so that all sequencer based commands could have a --status option I actually think we would want a "git sequencer" command, which can be fed an arbitrary instruction sheet created by a third-party and told to "run" it. A new command $cmd that wants to rewrite history (like "rebase -i", "cherry-pick A..B", etc. do) can only concentrate on preparing the sequence of instructions and then internally invoke "git sequencer run" until it gives the control back to the end user. When the user tells $cmd to continue, it can relay that request to "git sequencer continue" under the hood. Once its use is established, it might be even possible to let users run "git sequencer continue", bypassing frontends for individual commands, e.g. "git cherry-pick --continue", etc., but I do not know if that is generally a good idea or not. In any case, having such a front-end will help third-party scripts that want to build a custom workflow using the sequecing machinery we have. And in such a world, we would need "git sequencer status" command to give us where in a larger sequence of instrutions we are. So I tend to think this should be part of the core, not contrib/, and should become part of a new command "git sequencer".
Re: [BUG] rebase -i with only empty commits
On 08/23/2017 07:29 PM, Stefan Beller wrote: > On Wed, Aug 23, 2017 at 8:19 AM, Stephan Beyer wrote: >> On 08/23/2017 04:40 PM, Johannes Schindelin wrote: >>> These days, I reflexively type `rebase -ki` instead of `rebase -i`. Maybe >>> you want to do that, too? >> >> That's a very valuable hint, thank you very much! > > While -k side steps the original problem, it seems like it would > have helped me, too. > > Is there any value in discussing turning it on by default? I also wondered why empty commits are "discriminated" in such a way. I first thought that if you rebase branch A onto B but branch A and B contain commits with the same changes, then these commits would become new empty commits instead of simply being ignored. But I just checked this theory and it is now falsified :) It seems empty commits occur *only* if the user wants them to occur (--allow-empty). If they occur unintentionally (for example, by importing some SVN), one can eliminate them using filter-branch or rebase (by commenting out these picks). So it is still unclear to me, why empty commits are handled in such a special way. Best Stephan PS: Although -k helps, the original behavior of rebase -i is still a bug.
[GSoC][PATCH v2 0/4] submodule: Incremental rewrite of git-submodules
Changes in v2: * In the function get_submodule_displaypath(), I tried avoiding the extra memory allocation, but it rather invoked test 5 from t7406-submodule-update. The details of the test failiure can be seen in the build report as well. (Build #162) This failure occured as even though the function relative_path(), returned the value correctly, NULL was stored in sb.buf Hence currently the function is still using the old way of allocating extra memory and releasing the strbuf first, and return the extra allocated memory. * It was observed that strbuf_reset was being done just before the strbuf was being reused. It made more sense to reset them just after their use instead. Hence, these function calls of strbuf_reset() were moved accordingly. (The above change was limited to the function init_submodule() only, since already there was a deletion of one such funciton call, and IMO, it won't be suitable to carryout the operation for the complete file in the same commit.) * The function name for_each_submodule_list() was changed to for_each_submodule(). * Instead of passing the complete list to the function for_each_submodule(), only its pointer is passed to avoid the compiler from making copy of the list, and to make the code more efficient. * The function names of print_name_rev() and get_name_rev() were changed to get_rev_name() and compute_rev_name(). Now the function get_rev_name() acts as the front end of the subcommand and calls the function compute_rev_name() for generating and receiving the value of revision name. * The above change was also accompanied by the change in names of some variables used internally in the functions. As before you can find this series at: https://github.com/pratham-pc/git/commits/week-14-1 And the build report is available at: https://travis-ci.org/pratham-pc/git/builds/ Branch: week-14-1 Build #163 Prathamesh Chavan (4): submodule--helper: introduce get_submodule_displaypath() submodule--helper: introduce for_each_submodule() submodule: port set_name_rev() from shell to C submodule: port submodule subcommand 'status' from shell to C builtin/submodule--helper.c | 294 git-submodule.sh| 61 + 2 files changed, 273 insertions(+), 82 deletions(-) -- 2.13.0
[GSoC][PATCH v2 1/4] submodule--helper: introduce get_submodule_displaypath()
Introduce function get_submodule_displaypath() to replace the code
occurring in submodule_init() for generating displaypath of the
submodule with a call to it.
This new function will also be used in other parts of the system
in later patches.
Mentored-by: Christian Couder
Mentored-by: Stefan Beller
Signed-off-by: Prathamesh Chavan
---
builtin/submodule--helper.c | 38 +-
1 file changed, 25 insertions(+), 13 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 84562ec83..e666f84ba 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -220,6 +220,28 @@ static int resolve_relative_url_test(int argc, const char
**argv, const char *pr
return 0;
}
+static char *get_submodule_displaypath(const char *path, const char *prefix)
+{
+ const char *super_prefix = get_super_prefix();
+
+ if (prefix && super_prefix) {
+ BUG("cannot have prefix '%s' and superprefix '%s'",
+ prefix, super_prefix);
+ } else if (prefix) {
+ struct strbuf sb = STRBUF_INIT;
+ char *displaypath = xstrdup(relative_path(path, prefix, &sb));
+ strbuf_release(&sb);
+ return displaypath;
+ } else if (super_prefix) {
+ int len = strlen(super_prefix);
+ const char *format = is_dir_sep(super_prefix[len - 1]) ? "%s%s"
: "%s/%s";
+
+ return xstrfmt(format, super_prefix, path);
+ } else {
+ return xstrdup(path);
+ }
+}
+
struct module_list {
const struct cache_entry **entries;
int alloc, nr;
@@ -339,16 +361,7 @@ static void init_submodule(const char *path, const char
*prefix, int quiet)
/* Only loads from .gitmodules, no overlay with .git/config */
gitmodules_config();
-
- if (prefix && get_super_prefix())
- die("BUG: cannot have prefix and superprefix");
- else if (prefix)
- displaypath = xstrdup(relative_path(path, prefix, &sb));
- else if (get_super_prefix()) {
- strbuf_addf(&sb, "%s%s", get_super_prefix(), path);
- displaypath = strbuf_detach(&sb, NULL);
- } else
- displaypath = xstrdup(path);
+ displaypath = get_submodule_displaypath(path, prefix);
sub = submodule_from_path(&null_oid, path);
@@ -363,9 +376,9 @@ static void init_submodule(const char *path, const char
*prefix, int quiet)
* Set active flag for the submodule being initialized
*/
if (!is_submodule_active(the_repository, path)) {
- strbuf_reset(&sb);
strbuf_addf(&sb, "submodule.%s.active", sub->name);
git_config_set_gently(sb.buf, "true");
+ strbuf_reset(&sb);
}
/*
@@ -373,7 +386,6 @@ static void init_submodule(const char *path, const char
*prefix, int quiet)
* To look up the url in .git/config, we must not fall back to
* .gitmodules, so look it up directly.
*/
- strbuf_reset(&sb);
strbuf_addf(&sb, "submodule.%s.url", sub->name);
if (git_config_get_string(sb.buf, &url)) {
if (!sub->url)
@@ -410,9 +422,9 @@ static void init_submodule(const char *path, const char
*prefix, int quiet)
_("Submodule '%s' (%s) registered for path
'%s'\n"),
sub->name, url, displaypath);
}
+ strbuf_reset(&sb);
/* Copy "update" setting when it is not set yet */
- strbuf_reset(&sb);
strbuf_addf(&sb, "submodule.%s.update", sub->name);
if (git_config_get_string(sb.buf, &upd) &&
sub->update_strategy.type != SM_UPDATE_UNSPECIFIED) {
--
2.13.0
[GSoC][PATCH v2 2/4] submodule--helper: introduce for_each_submodule()
Introduce function for_each_submodule() and replace a loop
in module_init() with a call to it.
The new function will also be used in other parts of the
system in later patches.
Mentored-by: Christian Couder
Mentored-by: Stefan Beller
Signed-off-by: Prathamesh Chavan
---
builtin/submodule--helper.c | 39 +--
1 file changed, 29 insertions(+), 10 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index e666f84ba..847fba854 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -14,6 +14,9 @@
#include "refs.h"
#include "connect.h"
+typedef void (*submodule_list_func_t)(const struct cache_entry *list_item,
+ void *cb_data);
+
static char *get_default_remote(void)
{
char *dest = NULL, *ret;
@@ -353,17 +356,30 @@ static int module_list(int argc, const char **argv, const
char *prefix)
return 0;
}
-static void init_submodule(const char *path, const char *prefix, int quiet)
+static void for_each_submodule(const struct module_list *list,
+ submodule_list_func_t fn, void *cb_data)
+{
+ int i;
+ for (i = 0; i < list->nr; i++)
+ fn(list->entries[i], cb_data);
+}
+
+struct init_cb {
+ const char *prefix;
+ unsigned int quiet: 1;
+};
+#define INIT_CB_INIT { NULL, 0 }
+
+static void init_submodule(const struct cache_entry *list_item, void *cb_data)
{
+ struct init_cb *info = cb_data;
const struct submodule *sub;
struct strbuf sb = STRBUF_INIT;
char *upd = NULL, *url = NULL, *displaypath;
- /* Only loads from .gitmodules, no overlay with .git/config */
- gitmodules_config();
- displaypath = get_submodule_displaypath(path, prefix);
+ displaypath = get_submodule_displaypath(list_item->name, info->prefix);
- sub = submodule_from_path(&null_oid, path);
+ sub = submodule_from_path(&null_oid, list_item->name);
if (!sub)
die(_("No url found for submodule path '%s' in .gitmodules"),
@@ -375,7 +391,7 @@ static void init_submodule(const char *path, const char
*prefix, int quiet)
*
* Set active flag for the submodule being initialized
*/
- if (!is_submodule_active(the_repository, path)) {
+ if (!is_submodule_active(the_repository, list_item->name)) {
strbuf_addf(&sb, "submodule.%s.active", sub->name);
git_config_set_gently(sb.buf, "true");
strbuf_reset(&sb);
@@ -417,7 +433,7 @@ static void init_submodule(const char *path, const char
*prefix, int quiet)
if (git_config_set_gently(sb.buf, url))
die(_("Failed to register url for submodule path '%s'"),
displaypath);
- if (!quiet)
+ if (!info->quiet)
fprintf(stderr,
_("Submodule '%s' (%s) registered for path
'%s'\n"),
sub->name, url, displaypath);
@@ -446,10 +462,10 @@ static void init_submodule(const char *path, const char
*prefix, int quiet)
static int module_init(int argc, const char **argv, const char *prefix)
{
+ struct init_cb info = INIT_CB_INIT;
struct pathspec pathspec;
struct module_list list = MODULE_LIST_INIT;
int quiet = 0;
- int i;
struct option module_init_options[] = {
OPT__QUIET(&quiet, N_("Suppress output for initializing a
submodule")),
@@ -474,8 +490,11 @@ static int module_init(int argc, const char **argv, const
char *prefix)
if (!argc && git_config_get_value_multi("submodule.active"))
module_list_active(&list);
- for (i = 0; i < list.nr; i++)
- init_submodule(list.entries[i]->name, prefix, quiet);
+ info.prefix = prefix;
+ info.quiet = !!quiet;
+
+ gitmodules_config();
+ for_each_submodule(&list, init_submodule, &info);
return 0;
}
--
2.13.0
[GSoC][PATCH v2 3/4] submodule: port set_name_rev() from shell to C
Function set_name_rev() is ported from git-submodule to the
submodule--helper builtin. The function compute_rev_name() generates the
value of the revision name as required.
The function get_rev_name() calls compute_rev_name() and receives the
revision name, and later handles its formating and printing.
Mentored-by: Christian Couder
Mentored-by: Stefan Beller
Signed-off-by: Prathamesh Chavan
---
builtin/submodule--helper.c | 63 +
git-submodule.sh| 16 ++--
2 files changed, 65 insertions(+), 14 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 847fba854..6ae93ce38 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -245,6 +245,68 @@ static char *get_submodule_displaypath(const char *path,
const char *prefix)
}
}
+static char *compute_rev_name(const char *sub_path, const char* object_id)
+{
+ struct strbuf sb = STRBUF_INIT;
+ const char ***d;
+
+ static const char *describe_bare[] = {
+ NULL
+ };
+
+ static const char *describe_tags[] = {
+ "--tags", NULL
+ };
+
+ static const char *describe_contains[] = {
+ "--contains", NULL
+ };
+
+ static const char *describe_all_always[] = {
+ "--all", "--always", NULL
+ };
+
+ static const char **describe_argv[] = {
+ describe_bare, describe_tags, describe_contains,
+ describe_all_always, NULL
+ };
+
+ for (d = describe_argv; *d; d++) {
+ struct child_process cp = CHILD_PROCESS_INIT;
+ prepare_submodule_repo_env(&cp.env_array);
+ cp.dir = sub_path;
+ cp.git_cmd = 1;
+ cp.no_stderr = 1;
+
+ argv_array_push(&cp.args, "describe");
+ argv_array_pushv(&cp.args, *d);
+ argv_array_push(&cp.args, object_id);
+
+ if (!capture_command(&cp, &sb, 0) && sb.len) {
+ strbuf_strip_suffix(&sb, "\n");
+ return strbuf_detach(&sb, NULL);
+ }
+ }
+
+ strbuf_release(&sb);
+ return NULL;
+}
+
+static int get_rev_name(int argc, const char **argv, const char *prefix)
+{
+ char *revname;
+ if (argc != 3)
+ die("get-rev-name only accepts two arguments: ");
+
+ revname = compute_rev_name(argv[1], argv[2]);
+ if (revname && revname[0])
+ printf(" (%s)", revname);
+ printf("\n");
+
+ free(revname);
+ return 0;
+}
+
struct module_list {
const struct cache_entry **entries;
int alloc, nr;
@@ -1243,6 +1305,7 @@ static struct cmd_struct commands[] = {
{"relative-path", resolve_relative_path, 0},
{"resolve-relative-url", resolve_relative_url, 0},
{"resolve-relative-url-test", resolve_relative_url_test, 0},
+ {"get-rev-name", get_rev_name, 0},
{"init", module_init, SUPPORT_SUPER_PREFIX},
{"remote-branch", resolve_remote_submodule_branch, 0},
{"push-check", push_check, 0},
diff --git a/git-submodule.sh b/git-submodule.sh
index e131760ee..91f043ec6 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -759,18 +759,6 @@ cmd_update()
}
}
-set_name_rev () {
- revname=$( (
- sanitize_submodule_env
- cd "$1" && {
- git describe "$2" 2>/dev/null ||
- git describe --tags "$2" 2>/dev/null ||
- git describe --contains "$2" 2>/dev/null ||
- git describe --all --always "$2"
- }
- ) )
- test -z "$revname" || revname=" ($revname)"
-}
#
# Show commit summary for submodules in index or working tree
#
@@ -1042,14 +1030,14 @@ cmd_status()
fi
if git diff-files --ignore-submodules=dirty --quiet --
"$sm_path"
then
- set_name_rev "$sm_path" "$sha1"
+ revname=$(git submodule--helper get-rev-name "$sm_path"
"$sha1")
say " $sha1 $displaypath$revname"
else
if test -z "$cached"
then
sha1=$(sanitize_submodule_env; cd "$sm_path" &&
git rev-parse --verify HEAD)
fi
- set_name_rev "$sm_path" "$sha1"
+ revname=$(git submodule--helper get-rev-name "$sm_path"
"$sha1")
say "+$sha1 $displaypath$revname"
fi
--
2.13.0
[GSoC][PATCH v2 4/4] submodule: port submodule subcommand 'status' from shell to C
This aims to make git-submodule 'status' a built-in. Hence, the function
cmd_status() is ported from shell to C. This is done by introducing
three functions: module_status(), submodule_status() and print_status().
The function module_status() acts as the front-end of the subcommand.
It parses subcommand's options and then calls the function
module_list_compute() for computing the list of submodules. Then
this functions calls for_each_submodule() looping through the
list obtained.
Then for_each_submodule() calls submodule_status() for each of the
submodule in its list. The function submodule_status() is responsible
for generating the status each submodule it is called for, and
then calls print_status().
Finally, the function print_status() handles the printing of submodule's
status.
Mentored-by: Christian Couder
Mentored-by: Stefan Beller
Signed-off-by: Prathamesh Chavan
---
builtin/submodule--helper.c | 156
git-submodule.sh| 49 +-
2 files changed, 157 insertions(+), 48 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6ae93ce38..933073251 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -561,6 +561,161 @@ static int module_init(int argc, const char **argv, const
char *prefix)
return 0;
}
+struct status_cb {
+ const char *prefix;
+ unsigned int quiet: 1;
+ unsigned int recursive: 1;
+ unsigned int cached: 1;
+};
+#define STATUS_CB_INIT { NULL, 0, 0, 0 }
+
+static void print_status(struct status_cb *info, char state, const char *path,
+const struct object_id *oid, const char *displaypath)
+{
+ if (info->quiet)
+ return;
+
+ printf("%c%s %s", state, oid_to_hex(oid), displaypath);
+
+ if (state == ' ' || state == '+') {
+ struct argv_array get_rev_args = ARGV_ARRAY_INIT;
+
+ argv_array_pushl(&get_rev_args, "get-rev-name",
+path, oid_to_hex(oid), NULL);
+ get_rev_name(get_rev_args.argc, get_rev_args.argv,
+info->prefix);
+
+ argv_array_clear(&get_rev_args);
+ } else {
+ printf("\n");
+ }
+}
+
+static int handle_submodule_head_ref(const char *refname,
+const struct object_id *oid, int flags,
+void *cb_data)
+{
+ struct object_id *output = cb_data;
+ if (oid)
+ oidcpy(output, oid);
+
+ return 0;
+}
+
+static void status_submodule(const struct cache_entry *list_item, void
*cb_data)
+{
+ struct status_cb *info = cb_data;
+ char *displaypath;
+ struct argv_array diff_files_args = ARGV_ARRAY_INIT;
+
+ if (!submodule_from_path(&null_oid, list_item->name))
+ die(_("no submodule mapping found in .gitmodules for path
'%s'"),
+ list_item->name);
+
+ displaypath = get_submodule_displaypath(list_item->name, info->prefix);
+
+ if (ce_stage(list_item)) {
+ print_status(info, 'U', list_item->name,
+&null_oid, displaypath);
+ goto cleanup;
+ }
+
+ if (!is_submodule_active(the_repository, list_item->name)) {
+ print_status(info, '-', list_item->name, &list_item->oid,
+displaypath);
+ goto cleanup;
+ }
+
+ argv_array_pushl(&diff_files_args, "diff-files",
+"--ignore-submodules=dirty", "--quiet", "--",
+list_item->name, NULL);
+
+ if (!cmd_diff_files(diff_files_args.argc, diff_files_args.argv,
+ info->prefix)) {
+ print_status(info, ' ', list_item->name, &list_item->oid,
+displaypath);
+ } else {
+ if (!info->cached) {
+ struct object_id oid;
+
+ if (head_ref_submodule(list_item->name,
+ handle_submodule_head_ref, &oid))
+ die(_("could not resolve HEAD ref inside the"
+ "submodule '%s'"), list_item->name);
+
+ print_status(info, '+', list_item->name, &oid,
+displaypath);
+ } else {
+ print_status(info, '+', list_item->name,
+&list_item->oid, displaypath);
+ }
+ }
+
+ if (info->recursive) {
+ struct child_process cpr = CHILD_PROCESS_INIT;
+
+ cpr.git_cmd = 1;
+ cpr.dir = list_item->name;
+ prepare_submodule_repo_env(&cpr.env_array);
+
+ argv_array_pushl(&cpr.args, "--super-prefix", displaypath,
+"submodule
Re: [BUG] rebase -i with only empty commits
Stephan Beyer writes: > On 08/23/2017 07:29 PM, Stefan Beller wrote: >> On Wed, Aug 23, 2017 at 8:19 AM, Stephan Beyer wrote: >>> On 08/23/2017 04:40 PM, Johannes Schindelin wrote: These days, I reflexively type `rebase -ki` instead of `rebase -i`. Maybe you want to do that, too? >>> >>> That's a very valuable hint, thank you very much! >> >> While -k side steps the original problem, it seems like it would >> have helped me, too. >> >> Is there any value in discussing turning it on by default? As we often see on this list, people tend to be blinded by their own needs and fail to strike the right balance when considering the pros and cons of a change that may help their immediate desire, I think there is a value in discussing it, even if the conclusion turns out that it is not a good idea to change the default. Rather, make that "even if" "especially if"---we would reach a reasonable balance between pros and cons and won't have to repeat the discussion. > I also wondered why empty commits are "discriminated" in such a way. > I first thought that if you rebase branch A onto B but branch A and B > contain commits with the same changes, then these commits would become > new empty commits instead of simply being ignored. But I just checked > this theory and it is now falsified :) If you add an identical patch on each of your two toy example branches and try to rebase one over the other, an early logic that culls the set of commits to replay based on patch ID will remove them so you would not even see "it has become empty". I think you can redo your example by having two consecutive commits on one branch, and then make one commit on the other branch whose effect is those two commits' effect combined. Then rebase the latter on top of the former. "cherry-pick A..B" seems to be aware of this more interesting case and allows you to control the behaviour by having --keep-redundant-commits and --allow-empty as two separate options, but it seems there is no similar provision in "rebase".
Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf
Martin Ågren writes: > On 23 August 2017 at 19:24, Junio C Hamano wrote: >> Martin Ågren writes: >> >>> strbuf_setlen(., 0) writes '\0' to sb.buf[0], where buf is either >>> allocated and unique to sb, or the global slopbuf. The slopbuf is meant >>> to provide a guarantee that buf is not NULL and that a freshly >>> initialized buffer contains the empty string, but it is not supposed to >>> be written to. That strbuf_setlen writes to slopbuf has at least two >>> implications: >>> >>> First, it's wrong in principle. Second, it might be hiding misuses which >>> are just waiting to wreak havoc. Third, ThreadSanitizer detects a race >>> when multiple threads write to slopbuf at roughly the same time, thus >>> potentially making any more critical races harder to spot. >> >> There are two hard things in computer science ;-). > > Indeed. :-) > >>> Suggested-by: Junio C Hamano >>> Signed-off-by: Martin Ågren >>> --- >>> v2: no "ifdef TSAN"; moved check from strbuf_reset into strbuf_setlen >> >> Looks much better. I have a mild objection to "suggested-by", >> though. It makes it sound as if this were my itch, but it is not. >> >> All the credit for being motivate to fix the issue should go to you. >> For what I did during the review of the previous one to lead to this >> simpler version, if you want to document it, "helped-by" would be >> more appropriate. > > Ok, so that's two things to tweak in the commit message. I'll hold off > on v3 in case I get some more feedback the coming days. Thanks. Well, this one is good enough and your "at least two" is technically fine ;-) Let's not reroll this any further.
Re: [PATCH 1/2] Documentation/user-manual: update outdated example output
Martin Ågren writes:
> Since commit f7673490 ("more terse push output", 2007-11-05), git push
> has a completely different output format than the one shown in the user
> manual for a non-fast-forward push.
>
> Signed-off-by: Martin Ågren
> ---
> I'd say it's "not very many read this and immediately tried it out" and
> not "nobody read this for the last ten years".
Thanks for spotting, and I tend to agree that the above is very
close to a fair assessment.
When the software was young, those who adopted early were much more
highly motivated to not just follow documentation and examples but
also to improve them, compared to the more recent crop of new users
who can take it granted that "other people" work on supplying tools
and learning material for free. And that is not a bad thing---it is
progress.
So it may even be "many may have read, tried and found it different
from reality, but did not bother trying to fix or even report".
> Documentation/user-manual.txt | 10 ++
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt
> index bc2929867..d3c53b513 100644
> --- a/Documentation/user-manual.txt
> +++ b/Documentation/user-manual.txt
> @@ -2044,10 +2044,12 @@ If a push would not result in a
> <> of the
> remote branch, then it will fail with an error like:
>
> -
> -error: remote 'refs/heads/master' is not an ancestor of
> - local 'refs/heads/master'.
> - Maybe you are not up-to-date and need to pull first?
> -error: failed to push to 'ssh://yourserver.com/~you/proj.git'
> + ! [rejected]master -> master (non-fast-forward)
> +error: failed to push some refs to '...'
> +hint: Updates were rejected because the tip of your current branch is behind
> +hint: its remote counterpart. Integrate the remote changes (e.g.
> +hint: 'git pull ...') before pushing again.
> +hint: See the 'Note about fast-forwards' in 'git push --help' for details.
> -
>
> This can happen, for example, if you:
Re: [GSoC][PATCH v2 2/4] submodule--helper: introduce for_each_submodule()
Prathamesh Chavan writes:
> +typedef void (*submodule_list_func_t)(const struct cache_entry *list_item,
> + void *cb_data);
> +
> static char *get_default_remote(void)
> {
> char *dest = NULL, *ret;
> @@ -353,17 +356,30 @@ static int module_list(int argc, const char **argv,
> const char *prefix)
> return 0;
> }
>
> -static void init_submodule(const char *path, const char *prefix, int quiet)
> +static void for_each_submodule(const struct module_list *list,
> +submodule_list_func_t fn, void *cb_data)
In the output from
$ git grep for_each \*.h
we find that the convention is that an interator over a group of X
is for_each_X, the callback function that is given to for_each_X is
of type each_X_fn. An interator over a subset of group of X that
has trait Y, for_each_Y_X() iterates and calls back a function of
type each_X_fn (e.g. for_each_tag_ref() still calls each_ref_fn).
I do not offhand think of a reason why the above code need to
deviate from that pattern.
Re: [PATCH v4 02/16] refs.c: use is_dir_sep() in resolve_gitlink_ref()
On Wed, Aug 23, 2017 at 5:36 AM, Nguyễn Thái Ngọc Duy wrote: > The "submodule" argument in this function is a path, which can have > either '/' or '\\' as a separator. Use is_dir_sep() to support both. > > Noticed-by: Johannes Sixt > Signed-off-by: Nguyễn Thái Ngọc Duy Immediate questions that come to mind: * Could this go in as an independent bug fix? -> If so do we have any test that fails or stops failing on Windows? -> If not, do we need one? * Assuming this is not an independent bug fix: Why do we need this patch in this series here? (I thought this is about worktrees, not submodules. So does this fix a potential bug that will be exposed later in this series, but was harmless up to now?) I'll read the next patches to see if I will be enlightened. Thanks, Stefan > --- > refs.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/refs.c b/refs.c > index 3d549a8970..dec899a57a 100644 > --- a/refs.c > +++ b/refs.c > @@ -1507,7 +1507,7 @@ int resolve_gitlink_ref(const char *submodule, const > char *refname, > struct ref_store *refs; > int flags; > > - while (len && submodule[len - 1] == '/') > + while (len && is_dir_sep(submodule[len - 1])) > len--; > > if (!len) > -- > 2.11.0.157.gd943d85 >
Re: [PATCH v4 04/16] revision.c: --indexed-objects add objects from all worktrees
On Wed, Aug 23, 2017 at 5:36 AM, Nguyễn Thái Ngọc Duy wrote:
> This is the result of single_worktree flag never being set (no way to up
> until now). To get objects from current index only, set single_worktree.
>
> The other add_index_objects_to_pending's caller is mark_reachable_objects()
> (e.g. "git prune") which also mark objects from all indexes.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> revision.c | 21 +
> t/t5304-prune.sh | 9 +
> 2 files changed, 30 insertions(+)
>
> diff --git a/revision.c b/revision.c
> index 6c46ad6c8a..f35cb49af5 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -19,6 +19,7 @@
> #include "dir.h"
> #include "cache-tree.h"
> #include "bisect.h"
> +#include "worktree.h"
>
> volatile show_early_output_fn_t show_early_output;
>
> @@ -1290,8 +1291,28 @@ static void do_add_index_objects_to_pending(struct
> rev_info *revs,
>
> void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
> {
> + struct worktree **worktrees, **p;
> +
> read_cache();
> do_add_index_objects_to_pending(revs, &the_index);
> +
> + if (revs->single_worktree)
> + return;
> +
> + worktrees = get_worktrees(0);
> + for (p = worktrees; *p; p++) {
> + struct worktree *wt = *p;
> + struct index_state istate = { NULL };
> +
> + if (wt->is_current)
> + continue; /* current index already taken care of */
> +
> + if (read_index_from(&istate,
> + worktree_git_path(wt, "index")) > 0)
> + do_add_index_objects_to_pending(revs, &istate);
> + discard_index(&istate);
> + }
> + free_worktrees(worktrees);
> }
The commit (message and code) looks acceptable and easy to read.
I wonder though if another approach would be more feasible:
* factor out anything after the early return into a new function
"add_wt_index_objects_to_pending", maybe?
* introduce a new function
for_each_wt_except_current(function, callback_cookie)
such that this code could use this predefined iterator.
(I have the impression I have seen this code pattern before,
hence it may be useful to have such a foreach function; we
have foreach functions already for various ref things, string
list items, reflogs, and soon submodule lists)
>
> static int add_parents_only(struct rev_info *revs, const char *arg_, int
> flags,
> diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
> index 133b5842b1..cba45c7be9 100755
> --- a/t/t5304-prune.sh
> +++ b/t/t5304-prune.sh
> @@ -283,4 +283,13 @@ test_expect_success 'prune: handle alternate object
> database' '
> git -C B prune
> '
>
> +test_expect_success 'prune: handle index in multiple worktrees' '
> + git worktree add second-worktree &&
> + echo "new blob for second-worktree" >second-worktree/blob &&
> + git -C second-worktree add blob &&
> + git prune --expire=now &&
> + git -C second-worktree show :blob >actual &&
> + test_cmp second-worktree/blob actual
> +'
> +
> test_done
> --
> 2.11.0.157.gd943d85
>
Re: [GSoC][PATCH v2 2/4] submodule--helper: introduce for_each_submodule()
On Wed, Aug 23, 2017 at 12:13 PM, Junio C Hamano wrote:
> Prathamesh Chavan writes:
>
>> +typedef void (*submodule_list_func_t)(const struct cache_entry *list_item,
>> + void *cb_data);
>> +
>> static char *get_default_remote(void)
>> {
>> char *dest = NULL, *ret;
>> @@ -353,17 +356,30 @@ static int module_list(int argc, const char **argv,
>> const char *prefix)
>> return 0;
>> }
>>
>> -static void init_submodule(const char *path, const char *prefix, int quiet)
>> +static void for_each_submodule(const struct module_list *list,
>> +submodule_list_func_t fn, void *cb_data)
>
> In the output from
>
> $ git grep for_each \*.h
>
> we find that the convention is that an interator over a group of X
> is for_each_X,
... which this is...
> the callback function that is given to for_each_X is
> of type each_X_fn.
So you suggest s/submodule_list_func_t/each_submodule_fn/
> An interator over a subset of group of X that
> has trait Y, for_each_Y_X() iterates and calls back a function of
> type each_X_fn (e.g. for_each_tag_ref() still calls each_ref_fn).
This reads as a suggestion for for_each_listed_submodule
as the name.
> I do not offhand think of a reason why the above code need to
> deviate from that pattern.
Re: [PATCH v4 05/16] refs.c: refactor get_submodule_ref_store(), share common free block
On Wed, Aug 23, 2017 at 5:36 AM, Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> refs.c | 17 ++---
> 1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index dec899a57a..522c4ab74f 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1636,7 +1636,6 @@ struct ref_store *get_submodule_ref_store(const char
> *submodule)
> {
> struct strbuf submodule_sb = STRBUF_INIT;
> struct ref_store *refs;
Do we need to assign NULL here, to cover the early outs?
> - int ret;
>
> if (!submodule || !*submodule) {
> /*
> @@ -1648,19 +1647,14 @@ struct ref_store *get_submodule_ref_store(const char
> *submodule)
>
> refs = lookup_ref_store_map(&submodule_ref_stores, submodule);
Answering the question from above, no we do not need to
assign NULL as we have an unconditional assignment
here and any early outs are after this.
Thanks,
Stefan
Should rerere auto-update a merge resolution?
Hi List!
Let's say...
- git v2.9.4
- rerere is enabled.
- I merge maint into master, resolve erroneously, commit
- I publish my merge in a temp branch, a reviewer points out my mistake
- I reset hard, retry the merge, using --no-commit, rerere applies
what it knows
- I fix things up, then commit
So far so good.
Oops! One of the branches has moved forward in the meantime, so
- git fetch
- git reset --hard master
- git merge maint
... rerere applies the first (incorrect) resolution...
Am I doing it wrong? {C,Sh}ould rerere have done better?
cheers,
m
--
martin.langh...@gmail.com
- ask interesting questions ~ http://linkedin.com/in/martinlanghoff
- don't be distracted~ http://github.com/martin-langhoff
by shiny stuff
Re: Cannot checkout after setting the eol attribute
On Tue, Aug 22, 2017 at 03:44:41PM -0400, Ben Boeckel wrote: > On Tue, Aug 22, 2017 at 21:13:18 +0200, Torsten Bögershausen wrote: > > When you set the text attribute (in your case "eol=crlf" implies text) > > then the file(s) -must- be nomalized and commited so that they have LF > > in the repo (technically speaking the index) > > This seems like a special case that Git could detect and message about > somehow. > > > This is what is written about the "eol=crlf" attribute: > > This setting forces Git to normalize line endings for this > > file on checkin and convert them to CRLF when the file is > > checked out. > > And this is what is implemented in Git. > > Yeah, I read the docs, but the oddities of reset not doing its job > wasn't clear from this sentence :) . git reset does it's job - please see below. The problem is that we need a "git commit" here. After applying .gitattributes, it may be neccessary to "normalize" the files. If there is something in the documentation, that can be improved, please let us know. > > > Long story short: > > > > The following would solve your problem: > >git init > >echo $'dos\r' > dos > >git add dos > >git commit -m "dos newlines" > >echo "dos -crlf" > .gitattributes > >git add .gitattributes > >git commit -m "add attributes" > >echo "dos eol=crlf" > .gitattributes > >git read-tree --empty # Clean index, force re-scan of working directory > > The fact that plumbing is necessary to dig yourself out of a hole of the > `eol` attribute changes points to something needing to be changed, even > if it's only documentation. Could Git detect this and message about it > somehow when `git reset` cannot fix the working tree? The thing is, that the working tree is "in a good state": We want "dos" with CRLF, and that is what we have. There is nothing that can be improved in the working tree. What needs to be fixed, is the index. And that needs to be done with "git add" "git commit." As Junio pointed out, the read-tree is not ideal (to fix a single file in a possible dirty working tree) In your case it looks like this: echo "dos eol=crlf" > .gitattributes git add .gitattributes && git rm --cached dos && git add dos && git commit > Or maybe it could at least exit with failure instead of success? I don't know. It -may- be possible to add a warning in "git reset". I can have a look at that...
Re: [PATCH v4 10/16] refs: remove dead for_each_*_submodule()
On Wed, Aug 23, 2017 at 5:36 AM, Nguyễn Thái Ngọc Duy wrote: > These are used in revision.c. After the last patch they are replaced > with the refs_ version. Delete them. > > Signed-off-by: Nguyễn Thái Ngọc Duy The refactoring up to this patch looks good to me. Thanks, Stefan
Re: [GSoC][PATCH v2 2/4] submodule--helper: introduce for_each_submodule()
Stefan Beller writes:
> On Wed, Aug 23, 2017 at 12:13 PM, Junio C Hamano wrote:
>> Prathamesh Chavan writes:
>>
>>> +typedef void (*submodule_list_func_t)(const struct cache_entry *list_item,
>>> + void *cb_data);
>>> +
>>> static char *get_default_remote(void)
>>> {
>>> char *dest = NULL, *ret;
>>> @@ -353,17 +356,30 @@ static int module_list(int argc, const char **argv,
>>> const char *prefix)
>>> return 0;
>>> }
>>>
>>> -static void init_submodule(const char *path, const char *prefix, int quiet)
>>> +static void for_each_submodule(const struct module_list *list,
>>> +submodule_list_func_t fn, void *cb_data)
>>
>> In the output from
>>
>> $ git grep for_each \*.h
>>
>> we find that the convention is that an interator over a group of X
>> is for_each_X,
>
> ... which this is...
>
>> the callback function that is given to for_each_X is
>> of type each_X_fn.
>
> So you suggest s/submodule_list_func_t/each_submodule_fn/
It's not _I_ suggest---the remainder of the codebase screams that
the above name is wrong ;-). Didn't it for the mentors while they
were reading this code?
>> An interator over a subset of group of X that
>> has trait Y, for_each_Y_X() iterates and calls back a function of
>> type each_X_fn (e.g. for_each_tag_ref() still calls each_ref_fn).
>
> This reads as a suggestion for for_each_listed_submodule
> as the name.
If you need to have two ways to iterate over them, i.e. (1) over all
submodules and (2) over only the listed ones (whatever that means),
then yes, for_each_listed_submodule() would be a good name for the
latter which would be a complement to for_each_submodule() that is
the former.
>
>> I do not offhand think of a reason why the above code need to
>> deviate from that pattern.
Re: [PATCH v4 11/16] revision.c: --all adds HEAD from all worktrees
> +int other_head_refs(each_ref_fn fn, void *cb_data)
> +{
> + struct worktree **worktrees, **p;
> + int ret = 0;
> +
> + worktrees = get_worktrees(0);
> + for (p = worktrees; *p; p++) {
> + struct worktree *wt = *p;
> + struct ref_store *refs;
> +
> + if (wt->is_current)
> + continue;
As said in an earlier patch (and now this pattern
coming up twice in this patch series alone), the lines
of this function up to here, could become
part of a worktree iterator function?
> + refs = get_worktree_ref_store(wt);
> + ret = refs_head_ref(refs, fn, cb_data);
> + if (ret)
> + break;
with these 4 lines in the callback function.
> +/*
> + * Similar to head_ref() for all HEADs _except_ one from the current
> + * worktree, which is covered by head_ref().
> + */
> +int other_head_refs(each_ref_fn fn, void *cb_data);
This is already such an iterator function, just at another
abstraction level.
Re: [PATCH v3 1/4] imap-send: return with error if curl failed
Nicolas Morey-Chaisemartin writes: > curl_append_msgs_to_imap always returned 0, whether curl failed or not. > Return a proper status so git imap-send will exit with an error code > if womething wrong happened. womething? No need to resend only to fix if this is typo s/wome/some/. > > Signed-off-by: Nicolas Morey-Chaisemartin > --- > imap-send.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/imap-send.c b/imap-send.c > index b2d0b849b..09f29ea95 100644 > --- a/imap-send.c > +++ b/imap-send.c > @@ -1490,7 +1490,7 @@ static int curl_append_msgs_to_imap(struct > imap_server_conf *server, > curl_easy_cleanup(curl); > curl_global_cleanup(); > > - return 0; > + return res == CURLE_OK; > } > #endif
Re: [PATCH v3 1/4] imap-send: return with error if curl failed
Nicolas Morey-Chaisemartin writes: > curl_append_msgs_to_imap always returned 0, whether curl failed or not. > Return a proper status so git imap-send will exit with an error code > if womething wrong happened. > > Signed-off-by: Nicolas Morey-Chaisemartin > --- > imap-send.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/imap-send.c b/imap-send.c > index b2d0b849b..09f29ea95 100644 > --- a/imap-send.c > +++ b/imap-send.c > @@ -1490,7 +1490,7 @@ static int curl_append_msgs_to_imap(struct > imap_server_conf *server, > curl_easy_cleanup(curl); > curl_global_cleanup(); > > - return 0; > + return res == CURLE_OK; > } > #endif Wait a bit. Did you mean "res != CURLE_OK"? If we got an OK, we want to return 0 as success, because the value we return from here is returned by cmd_main() as-is to main() and to the outside world, no?
Undocumented change in `git branch -M` behavior
Hello, Hopefully, I've got this right -- I noticed a change in behavior in git with Ubuntu 17.10, which recently got 2.14.1. Specifically, that when in an orphaned branch, -M ends up moving HEAD to the new branch name, clobbering the working tree. As far as I know, from the manpages, orphaned branches are still supported and should work? I think an example will demonstrate more than words (the following are done in LXD containers, hence the root user): # git --version git version 2.14.1 # mkdir test && cd test && git init . Initialized empty Git repository in /root/test/.git/ # git checkout -b a Switched to a new branch 'a' # touch testfile && git add testfile && git commit -m 'initial commit' [a (root-commit) 6061193] initial commit Committer: root 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 testfile # git checkout --orphan master Switched to a new branch 'master' # git status On branch master No commits yet Changes to be committed: (use "git rm --cached ..." to unstage) new file: testfile # git reset --hard && git status On branch master No commits yet nothing to commit (create/copy files and use "git add" to track) # git branch -M a b # git status On branch b Changes to be committed: (use "git reset HEAD ..." to unstage) deleted:testfile This is very unexpected. I force-renamed a branch I wasn't currently checked out to and now I'm checked out to it *and* I have staged file removals (I think what is effectively happening is my current working directory (empty) is being staged into the new branch, but I'm not 100%). For comparision, on 17.04: # git --version git version 2.11.0 # mkdir test && cd test && git init . Initialized empty Git repository in /root/test/.git/ # git checkout -b a Switched to a new branch 'a' # touch testfile && git add testfile && git commit -m 'initial commit' [a (root-commit) f8d0d53] initial commit Committer: root 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 testfile # git checkout --orphan master Switched to a new branch 'master' # git status On branch master No commits yet Changes to be committed: (use "git rm --cached ..." to unstage) new file: testfile # git reset --hard && git status On branch master No commits yet nothing to commit (create/copy files and use "git add" to track) # git branch -M a b # git status On branch master Initial commit nothing to commit (create/copy files and use "git add" to track) This is what I expect to see, the branch rename has no effect on HEAD. I haven't yet bisected this (but I can if necessary). My initial suspicion is https://github.com/git/git/commit/70999e9ceca47e03b8900bfb310b2f804125811e#diff-d18f86ea14e2f1e5bff391b2e54438cb where a comparison between the oldname of the branch and HEAD was performed before attempting to move HEAD (so that HEAD followed to the new branch name, I believe). That change was dropped, though and perhaps the new check in replace_each_worktree_head_symref of strcmp(oldref, worktrees[i]->head_ref) does not work for orphaned branches? I am unfamiliar with all the details of the git internals, so please correct me if I'm wrong! Thanks, Nish -- Nishanth Aravamudan Ubuntu Server Canonical Ltd
Re: [PATCH v3 2/4] imap-send: add wrapper to get server credentials if needed
Nicolas Morey-Chaisemartin writes:
> Signed-off-by: Nicolas Morey-Chaisemartin
> ---
> imap-send.c | 34 --
> 1 file changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/imap-send.c b/imap-send.c
> index 09f29ea95..448a4a0b3 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -926,6 +926,25 @@ static int auth_cram_md5(struct imap_store *ctx, struct
> imap_cmd *cmd, const cha
> return 0;
> }
>
> +static void server_fill_credential(struct imap_server_conf *srvc, struct
> credential *cred)
> +{
> + if (srvc->user && srvc->pass)
> + return;
> +
> + cred->protocol = xstrdup(srvc->use_ssl ? "imaps" : "imap");
> + cred->host = xstrdup(srvc->host);
> +
> + cred->username = xstrdup_or_null(srvc->user);
> + cred->password = xstrdup_or_null(srvc->pass);
> +
> + credential_fill(cred);
> +
> + if (!srvc->user)
> + srvc->user = xstrdup(cred->username);
> + if (!srvc->pass)
> + srvc->pass = xstrdup(cred->password);
> +}
> +
This looks straight-forward code movement. The only thing that
makes me wonder is if this is "server". The existing naming of the
variables screams at me that this is not "server" but is "service".
> static struct imap_store *imap_open_store(struct imap_server_conf *srvc,
> char *folder)
> {
> struct credential cred = CREDENTIAL_INIT;
> @@ -1078,20 +1097,7 @@ static struct imap_store *imap_open_store(struct
> imap_server_conf *srvc, char *f
> }
> #endif
> imap_info("Logging in...\n");
> - if (!srvc->user || !srvc->pass) {
> - cred.protocol = xstrdup(srvc->use_ssl ? "imaps" :
> "imap");
> - cred.host = xstrdup(srvc->host);
> -
> - cred.username = xstrdup_or_null(srvc->user);
> - cred.password = xstrdup_or_null(srvc->pass);
> -
> - credential_fill(&cred);
> -
> - if (!srvc->user)
> - srvc->user = xstrdup(cred.username);
> - if (!srvc->pass)
> - srvc->pass = xstrdup(cred.password);
> - }
> + server_fill_credential(srvc, &cred);
>
> if (srvc->auth_method) {
> struct imap_cmd_cb cb;
Re: [PATCH v3 4/4] imap-send: use curl by default
Nicolas Morey-Chaisemartin writes: > Now that curl is enable by default, s/enable/&d/; But it is unclear what the above really means. You certainly do not mean that [PATCH 1-3/4] somewhere tweaked our Makefile to always use libcurl and makes Git fail to build without it, but the above sounds as if that were the case. > use the curl implementation > for imap too. The Makefile for a long time by default set USE_CURL_FOR_IMAP_SEND to YesPlease when the version of cURL we have is recent enough, I think. So I am not sure what you want to add with this change. > The goal is to validate feature parity between the legacy and > the curl implementation, deprecate thee legacy implementation s/thee/the/; > later on and in the long term, hopefully drop it altogether. > > Signed-off-by: Nicolas Morey-Chaisemartin > --- > imap-send.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) Hmph, the patch itself is also confusing. > diff --git a/imap-send.c b/imap-send.c > index a74d011a9..58c191704 100644 > --- a/imap-send.c > +++ b/imap-send.c > @@ -35,11 +35,11 @@ typedef void *SSL; > #include "http.h" > #endif > > -#if defined(USE_CURL_FOR_IMAP_SEND) && defined(NO_OPENSSL) > -/* only available option */ > +#if defined(USE_CURL_FOR_IMAP_SEND) > +/* Always default to curl if it's available. */ > #define USE_CURL_DEFAULT 1 The original says "we want to use CURL, if Makefile tells us to *AND* if Makefile tells us not to use OpenSSL", which does not make much sense to me. I wonder if the original is buggy and should have been using "|| defined(NO_OPENSSL)" there instead. Perhaps that is the bug you are fixing with this patch, and all the talk about curl is default we saw above is a red herring? If that is the case, then instead of removing, turning "&&" into "||" may be a better fix. I dunno. > #else > -/* strictly opt in */ > +/* We don't have curl, so continue to use the historical implementation */ > #define USE_CURL_DEFAULT 0 > #endif
Re: Should rerere auto-update a merge resolution?
Martin Langhoff writes:
> Hi List!
>
> Let's say...
> - git v2.9.4
> - rerere is enabled.
> - I merge maint into master, resolve erroneously, commit
> - I publish my merge in a temp branch, a reviewer points out my mistake
> - I reset hard, retry the merge, using --no-commit, rerere applies
> what it knows
> - I fix things up, then commit
>
> So far so good.
>
> Oops! One of the branches has moved forward in the meantime, so
>
> - git fetch
> - git reset --hard master
> - git merge maint
> ... rerere applies the first (incorrect) resolution...
>
> Am I doing it wrong? {C,Sh}ould rerere have done better?
Between these two steps:
> - I reset hard, retry the merge, using --no-commit, rerere applies what it
> knows
> - I fix things up, then commit
You'd tell rerere to forget what it knows because it is wrong. Then
after these two (eh, now "three" because there is the "forget"
step), "rerere" notices that an updated resolution needs to be
recorded, so it remembers it. Later re-resolution will replay the
corrected one, simply because the old incorrect one is forgotten and
replaced by the updated correct one.
Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf
On Mon, Aug 21, 2017 at 10:43 AM, Martin Ågren wrote:
> strbuf_setlen(., 0) writes '\0' to sb.buf[0], where buf is either
> allocated and unique to sb, or the global slopbuf. The slopbuf is meant
> to provide a guarantee that buf is not NULL and that a freshly
> initialized buffer contains the empty string, but it is not supposed to
> be written to. That strbuf_setlen writes to slopbuf has at least two
> implications:
> diff --git a/strbuf.h b/strbuf.h
> index e705b94db..7496cb8ec 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -147,7 +147,10 @@ static inline void strbuf_setlen(struct strbuf *sb,
> size_t len)
> if (len > (sb->alloc ? sb->alloc - 1 : 0))
> die("BUG: strbuf_setlen() beyond buffer");
> sb->len = len;
> - sb->buf[len] = '\0';
> + if (sb->buf != strbuf_slopbuf)
> + sb->buf[len] = '\0';
> + else
> + assert(!strbuf_slopbuf[0]);
> }
>
> /**
> --
> 2.14.1.151.gdfeca7a7e
>
I know this must have been discussed before and/or I'm having a neuron
misfire, but is there any reason why we didn't just make slopbuf a
macro for ""?
i.e.
#define strbuf_slopbuf ""
That way it should point to readonly memory and any attempt to assign
to it should produce a crash.
One benefit that I can think of for making strbuf_slopbuf be a real
variable is that we can then compare the pointer stored in the strbuf
to the strbuf_slopbuf address to detect whether the strbuf held the
slopbuf. With the static string macro, each execution unit may get
it's own instance of the empty string. But, before this patch, we
don't actually seem to be doing that anywhere and instead rely on the
value of alloc being accurate to determine whether the strbuf contains
the slopbuf or not.
So is there any reason why didn't do something like the following in
the first place?
diff --git a/strbuf.h b/strbuf.h
index e705b94..fcca618 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -67,7 +67,7 @@ struct strbuf {
char *buf;
};
-extern char strbuf_slopbuf[];
+#define strbuf_slopbuf ""
#define STRBUF_INIT { .alloc = 0, .len = 0, .buf = strbuf_slopbuf }
/**
@@ -147,7 +147,9 @@ static inline void strbuf_setlen(struct strbuf
*sb, size_t len)
if (len > (sb->alloc ? sb->alloc - 1 : 0))
die("BUG: strbuf_setlen() beyond buffer");
sb->len = len;
- sb->buf[len] = '\0';
+ if (sb->alloc) {
+ sb->buf[len] = '\0';
+ }
}
-Brandon
Re: [PATCH v4 14/16] rev-list: expose and document --single-worktree
On Wed, Aug 23, 2017 at 5:37 AM, Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> Documentation/rev-list-options.txt | 8
> revision.c | 2 ++
> 2 files changed, 10 insertions(+)
>
> diff --git a/Documentation/rev-list-options.txt
> b/Documentation/rev-list-options.txt
> index a6cf9eb380..7d860bfca1 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -184,6 +184,14 @@ explicitly.
> Pretend as if all objects mentioned by reflogs are listed on the
> command line as ``.
>
> +--single-worktree::
> + By default, all working trees will be examined by the
> + following options when there are more than one (see
> + linkgit:git-worktree[1]): `--all`, `--reflog` and
> + `--indexed-objects`.
> + This option forces them to examine the current working tree
> + only.
> +
> --ignore-missing::
> Upon seeing an invalid object name in the input, pretend as if
> the bad input was not given.
> diff --git a/revision.c b/revision.c
> index d100b3a3be..6eba4131b4 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2251,6 +2251,8 @@ static int handle_revision_pseudo_opt(const char
> *submodule,
> return error("invalid argument to --no-walk");
> } else if (!strcmp(arg, "--do-walk")) {
> revs->no_walk = 0;
> + } else if (!strcmp(arg, "--single-worktree")) {
> + revs->single_worktree = 1;
This is in handle_revision_pseudo_opt, that has the note
/*
* NOTE!
*
* Commands like "git shortlog" will not accept the options below
* unless parse_revision_opt queues them (as opposed to erroring
* out).
*
* When implementing your new pseudo-option, remember to
* register it in the list at the top of handle_revision_opt.
*/
The registration needs to be done at around line 1700.
But come to think of it, is it really a pseudo opt?
Could it be a "real" (non pseudo) opt in handle_revision_opt?
The reasoning (either way) would be of interest in the
commit message, IMHO.
Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf
Brandon Casey writes:
> So is there any reason why didn't do something like the following in
> the first place?
My guess is that we didn't bother; if we cared, we would have used a
single instance of const char in a read-only segment, instead of
such a macro.
> diff --git a/strbuf.h b/strbuf.h
> index e705b94..fcca618 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -67,7 +67,7 @@ struct strbuf {
> char *buf;
> };
>
> -extern char strbuf_slopbuf[];
> +#define strbuf_slopbuf ""
> #define STRBUF_INIT { .alloc = 0, .len = 0, .buf = strbuf_slopbuf }
>
> /**
> @@ -147,7 +147,9 @@ static inline void strbuf_setlen(struct strbuf
> *sb, size_t len)
> if (len > (sb->alloc ? sb->alloc - 1 : 0))
> die("BUG: strbuf_setlen() beyond buffer");
> sb->len = len;
> - sb->buf[len] = '\0';
> + if (sb->alloc) {
> + sb->buf[len] = '\0';
> + }
> }
>
> -Brandon
Re: Cannot checkout after setting the eol attribute
On Wed, Aug 23, 2017 at 21:43:15 +0200, Torsten Bögershausen wrote: > git reset does it's job - please see below. > > The problem is that we need a "git commit" here. > After applying .gitattributes, it may be neccessary to "normalize" the > files. If there is something in the documentation, that can be > improved, please let us know. I'll have a patch up shortly. > On Tue, Aug 22, 2017 at 03:44:41PM -0400, Ben Boeckel wrote: > > The fact that plumbing is necessary to dig yourself out of a hole of the > > `eol` attribute changes points to something needing to be changed, even > > if it's only documentation. Could Git detect this and message about it > > somehow when `git reset` cannot fix the working tree? > > The thing is, that the working tree is "in a good state": > We want "dos" with CRLF, and that is what we have. > There is nothing that can be improved in the working tree. > What needs to be fixed, is the index. And that needs to be done with > "git add" "git commit." > As Junio pointed out, the read-tree is not ideal > (to fix a single file in a possible dirty working tree) > > In your case it looks like this: > > echo "dos eol=crlf" > .gitattributes > git add .gitattributes && > git rm --cached dos && git add dos && > git commit In this case, just adding the file should work: the file is on-disk as intended and adding the file should normalize the line endings when adding it into the index (basically, just `git add dos` should be required to make the index look like it should). > > Or maybe it could at least exit with failure instead of success? > > I don't know. > It -may- be possible to add a warning in "git reset". > I can have a look at that... Thanks. --Ben
Re: Should rerere auto-update a merge resolution?
On Wed, Aug 23, 2017 at 4:34 PM, Junio C Hamano wrote: > Between these two steps: > >> - I reset hard, retry the merge, using --no-commit, rerere applies what it >> knows >> - I fix things up, then commit > > You'd tell rerere to forget what it knows because it is wrong. Hi Junio! thanks for the quick response. Questions - when I tell it to forget, won't it forget the pre-resolution state? my read of the rerere docs imply that it gets called during the merge to record the conflicted state. - would it be a feature if it updated its resolution db automagically? rerere is plenty automagic already... cheers, m -- martin.langh...@gmail.com - ask interesting questions ~ http://linkedin.com/in/martinlanghoff - don't be distracted~ http://github.com/martin-langhoff by shiny stuff
[PATCH] Documentation: mention that `eol` can change the dirty status of paths
When setting the `eol` attribute, paths can change their dirty status without any change in the working directory. This can cause confusion and should at least be mentioned with a remedy. Signed-off-by: Ben Boeckel --- Documentation/gitattributes.txt | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt index c4f2be2..3044b71 100644 --- a/Documentation/gitattributes.txt +++ b/Documentation/gitattributes.txt @@ -151,7 +151,11 @@ unspecified. This attribute sets a specific line-ending style to be used in the working directory. It enables end-of-line conversion without any -content checks, effectively setting the `text` attribute. +content checks, effectively setting the `text` attribute. Note that +setting this attribute on paths which are in the index with different +line endings than the attribute indicates the paths to be considered +dirty. Adding the path to the index again will normalize the line +endings in the index. Set to string value "crlf":: -- 2.9.5
Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf
On Wed, Aug 23, 2017 at 2:04 PM, Junio C Hamano wrote:
> Brandon Casey writes:
>
>> So is there any reason why didn't do something like the following in
>> the first place?
>
> My guess is that we didn't bother; if we cared, we would have used a
> single instance of const char in a read-only segment, instead of
> such a macro.
I think you mean something like this:
const char * const strbuf_slopbuf = "";
..with or without "const" at the beginning. We can't use an actual
variable like that since we also want to be able to do initialization
like:
struct strbuf b = STRBUF_INIT;
i.e.
struct strbuf b = { 0, 0, strbuf_slopbuf };
So the compiler needs to be able to determine that everything within
the curly braces is constant and apparently gcc cannot.
On a related note... I was just looking at object.c which also uses a
slopbuf. We could similarly protect it from inadvertent modification
by doing something like this:
diff --git a/object.c b/object.c
index 321d7e9..4c7a041 100644
--- a/object.c
+++ b/object.c
@@ -303,7 +303,7 @@ int object_list_contains(struct object_list *list, struct ob
ject *obj)
* A zero-length string to which object_array_entry::name can be
* initialized without requiring a malloc/free.
*/
-static char object_array_slopbuf[1];
+static const char * const object_array_slopbuf = "";
void add_object_array_with_path(struct object *obj, const char *name,
struct object_array *array,
@@ -326,7 +326,7 @@ void add_object_array_with_path(struct object
*obj, const char *name,
entry->name = NULL;
else if (!*name)
/* Use our own empty string instead of allocating one: */
- entry->name = object_array_slopbuf;
+ entry->name = (char*) object_array_slopbuf;
else
entry->name = xstrdup(name);
entry->mode = mode;
Re: [PATCH] Documentation: mention that `eol` can change the dirty status of paths
On Wed, Aug 23, 2017 at 17:17:41 -0400, Ben Boeckel wrote: > diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt > index c4f2be2..3044b71 100644 > --- a/Documentation/gitattributes.txt > +++ b/Documentation/gitattributes.txt > @@ -151,7 +151,11 @@ unspecified. > > This attribute sets a specific line-ending style to be used in the > working directory. It enables end-of-line conversion without any > -content checks, effectively setting the `text` attribute. > +content checks, effectively setting the `text` attribute. Note that > +setting this attribute on paths which are in the index with different > +line endings than the attribute indicates the paths to be considered Oops, missed a "causes" in here. ---^ I'll wait on uploading a new patch until after feedback on this one. > +dirty. Adding the path to the index again will normalize the line > +endings in the index. --Ben
Re: [PATCH] Doc: clarify that pack-objects makes packs, plural
On Tue, Aug 22, 2017 at 12:56:25PM -0700, Junio C Hamano wrote: > - There should be an update to say max-pack-size is not something >normal users would ever want. Agreed. > diff --git a/Documentation/git-pack-objects.txt > b/Documentation/git-pack-objects.txt > index 8973510a41..3aa6234501 100644 > --- a/Documentation/git-pack-objects.txt > +++ b/Documentation/git-pack-objects.txt > @@ -108,9 +108,13 @@ base-name:: > is taken from the `pack.windowMemory` configuration variable. > > --max-pack-size=:: > - Maximum size of each output pack file. The size can be suffixed with > + In unusual scenarios, you may not be able to create files > + larger than certain size on your filesystem, and this option > + can be used to tell the command to split the output packfile > + into multiple independent packfiles and what the maximum > + size of each packfile is. The size can be suffixed with > "k", "m", or "g". The minimum size allowed is limited to 1 MiB. > - If specified, multiple packfiles may be created, which also > + This option > prevents the creation of a bitmap index. > The default is unlimited, unless the config variable > `pack.packSizeLimit` is set. I wonder if it is worth mentioning the other downside: that the sum of the split packfiles may be substantially larger than a single packfile would be (due to lost delta opportunities between the split packs). For the sneaker-net case, you are much better off generating a single pack and then using "split" and "cat" to reconstruct it on the other end Not that I think we should go into such detail in the manpage, but I have to wonder if --max-pack-size has outlived its usefulness. The only use case I can think of is a filesystem that cannot hold files larger than N bytes. -Peff
Re: What's cooking in git.git (Aug 2017, #05; Tue, 22)
On 22 Aug 2017, at 21:56, Junio C Hamano wrote:
> Here are the topics that have been cooking. Commits prefixed with
> '-' are only in 'pu' (proposed updates) while commits prefixed with
> '+' are in 'next'. The ones marked with '.' do not appear in any of
> the integration branches, but I am still holding onto them.
>
> The second batch of topics are in. This cycle is going a bit slower
> than the previous one (as of mid-week #3 of this cycle, we have
> about 200 patches on 'master' since v2.14, compared to about 300
> patches in the cycle towards v2.14 at a similar point in the cycle),
> but hopefully we can catch up eventually.
>
> I am planning to be offline most of the next week, by the way.
>
> You can find the changes described here in the integration branches
> of the repositories listed at
>
>http://git-blame.blogspot.com/p/git-public-repositories.html
>
> --
> [Graduated to "master"]
>
Hi Junio,
just in case this got lost: I posted a patch with an improvement to
2841e8f ("convert: add "status=delayed" to filter process protocol",
2017-06-30) which was merged to master in the beginning of 2.15.
https://public-inbox.org/git/20170820154720.32259-1-larsxschnei...@gmail.com/
Thanks,
Lars
Re: [RFC 0/3] imap-send curl tunnelling support
On Mon, Aug 21, 2017 at 09:34:19AM +0200, Nicolas Morey-Chaisemartin wrote: > >> It appears curl do not support the PREAUTH tag. > > Too bad. IMHO preauth is the main reason to use a tunnel in the first > > place. > > It shouldn't be too hard to add support for this in curl. > If it's the main usecase, it'll simply means the curl tunnelling > should be disabled by default for older curl (in this case, meaning > every version until it gets supported) versions. Yes, I agree. I was hoping when we started this discussion that we were more ready to switch to curl-by-default. But sadly, that isn't close to being the case. But hopefully we can at least end up with logic that lets us use it in the easy cases (no tunneling) and falls back in the harder ones. -Peff
Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf
On Wed, Aug 23, 2017 at 2:20 PM, Brandon Casey wrote:
> On Wed, Aug 23, 2017 at 2:04 PM, Junio C Hamano wrote:
>> Brandon Casey writes:
>>
>>> So is there any reason why didn't do something like the following in
>>> the first place?
>>
>> My guess is that we didn't bother; if we cared, we would have used a
>> single instance of const char in a read-only segment, instead of
>> such a macro.
>
> I think you mean something like this:
>
>const char * const strbuf_slopbuf = "";
Ah, you probably meant something like this:
const char strbuf_slopbuf = '\0';
which gcc will apparently place in the read-only segment. I did not know that.
And assignment and initialization would look like:
sb->buf = (char*) &strbuf_slopbuf;
and
#define STRBUF_INIT { .alloc = 0, .len = 0, .buf = (char*) &strbuf_slopbuf }
respectively. Yeah, that's definitely preferable to a macro.
Something similar could be done in object.c.
-Brandon
Re: [PATCH] Retry acquiring reference locks for 100ms
Michael Haggerty writes: > The philosophy of reference locking has been, "if another process is > changing a reference, then whatever I'm trying to do to it will > probably fail anyway because my old-SHA-1 value is probably no longer > current". But this argument falls down if the other process has locked > the reference to do something that doesn't actually change the value > of the reference, such as `pack-refs` or `reflog expire`. There > actually *is* a decent chance that a planned reference update will > still be able to go through after the other process has released the > lock. The reason why these 'read-only' operations take locks is because they want to ensure that other people will not mess with the anchoring points of the history they base their operation on while they do their work, right? > So when trying to lock an individual reference (e.g., when creating > "refs/heads/master.lock"), if it is already locked, then retry the > lock acquisition for approximately 100 ms before giving up. This > should eliminate some unnecessary lock conflicts without wasting a lot > of time. > > Add a configuration setting, `core.filesRefLockTimeout`, to allow this > setting to be tweaked. I suspect that this came from real-life needs of a server operator. What numbers should I be asking to justify this change? ;-) "Without this change, 0.4% of pushes used to fail due to losing the race against periodic GC, but with this, the rate went down to 0.2%, which is 50% improvement!" or something like that?
Re: [RFC PATCH 1/2] send-email: fix garbage removal after address
On Wed, Aug 23, 2017 at 3:21 AM, Matthieu Moy wrote: > This is a followup over 9d33439 (send-email: only allow one address > per body tag, 2017-02-20). The first iteration did allow writting > > Cc: # garbage > > but did so by matching the regex ([^>]*>?), i.e. stop after the first > instance of '>'. However, it did not properly deal with > > Cc: f...@example.com # garbage > > Fix this using a new function strip_garbage_one_address, which does > essentially what the old ([^>]*>?) was doing, but dealing with more > corner-cases. Since we've allowed > > Cc: "Foo # Bar" > > in previous versions, it makes sense to continue allowing it (but we > still remove any garbage after it). OTOH, when an address is given > without quoting, we just take the first word and ignore everything > after. > > Signed-off-by: Matthieu Moy > --- I pulled this and tested it for my issue, and it fixes the problem for me. I think the approach in the code was solid too, extracting out the logic helps make the code more clear. Thanks, Jake
Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf
On Wed, Aug 23, 2017 at 2:54 PM, Brandon Casey wrote: > On Wed, Aug 23, 2017 at 2:20 PM, Brandon Casey wrote: >> On Wed, Aug 23, 2017 at 2:04 PM, Junio C Hamano wrote: >>> Brandon Casey writes: >>> So is there any reason why didn't do something like the following in the first place? >>> >>> My guess is that we didn't bother; if we cared, we would have used a >>> single instance of const char in a read-only segment, instead of >>> such a macro. >> >> I think you mean something like this: >> >>const char * const strbuf_slopbuf = ""; Hmm, apparently it is sufficient to mark our current strbuf_slopbuf array as const and initialize it with a static string to trigger its placement into the read-only section by gcc (and clang). const char strbuf_slopbuf[1] = ""; -Brandon
Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf
Brandon Casey writes: > On Wed, Aug 23, 2017 at 2:04 PM, Junio C Hamano wrote: >> Brandon Casey writes: >> >>> So is there any reason why didn't do something like the following in >>> the first place? >> >> My guess is that we didn't bother; if we cared, we would have used a >> single instance of const char in a read-only segment, instead of >> such a macro. > > I think you mean something like this: > >const char * const strbuf_slopbuf = ""; Rather, more like "const char slopbuf[1];"
Re: [RFC 0/3] imap-send curl tunnelling support
Hi Hannes, On Tue, 22 Aug 2017, Johannes Sixt wrote: > Am 21.08.2017 um 09:27 schrieb Nicolas Morey-Chaisemartin: > > (Sent a reply from my phone while out of town but couldn't find it so here > > it is again) > > > > It's available on my github: > > https://github.com/nmorey/git/tree/dev/curl-tunnel > > > > The series had been stlighly changed since the patch were posted, mostly to > > add the proper ifdefs to handle older curl versions. > > This does not build for me on Windows due to a missing socketpair() function. > But I am working in an old environment, so I do not know whether this > statement has much value. Same problem in Git for Windows' SDK: imap-send.c: In function 'setup_tunnel': imap-send.c:936:6: error: implicit declaration of function 'socketpair'; did you mean 'socket'? [-Werror=implicit-function-declaration] if (socketpair(AF_UNIX, SOCK_STREAM, 0, sock_fds)) ^~ socket imap-send.c: In function 'curl_tunnel_socket': imap-send.c:1416:9: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast] return (unsigned long)clientp; ^
Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf
On Wed, Aug 23, 2017 at 3:24 PM, Junio C Hamano wrote: > Brandon Casey writes: > >> On Wed, Aug 23, 2017 at 2:04 PM, Junio C Hamano wrote: >>> Brandon Casey writes: >>> So is there any reason why didn't do something like the following in the first place? >>> >>> My guess is that we didn't bother; if we cared, we would have used a >>> single instance of const char in a read-only segment, instead of >>> such a macro. >> >> I think you mean something like this: >> >>const char * const strbuf_slopbuf = ""; > > Rather, more like "const char slopbuf[1];" You'd think that would be sufficient right? Apparently gcc and clang will only place a variable marked as const in the read-only section if you initialize it with a constant too. (gcc 4.9.2, clang 3.8.1 on linux, clang 8.1.0 on osx) I might have to adjust my stance on initializing global variables moving forward :-) -Brandon
Re: [BUG] rebase -i with only empty commits
From: "Johannes Schindelin" So the problem seems to be that rebase -i (like rebase without -i) considers "empty commits" as commits to be ignored. However, when using rebase -i one expects that you can include the empty commit... Also, the behavior is odd. When I only have empty commits, a "git rebase master" works as expected like a "git reset --hard master" but "git rebase -i" does nothing. The expected behavior would be that the editor shows up with a git-rebase-todo like: # pick 3d0f6c49 empty commit # pick bbbc5941 another empty commit noop These days, I reflexively type `rebase -ki` instead of `rebase -i`. Maybe you want to do that, too? Ciao, Dscho Is the -k option actually documented? I couldn't see it in the man pages. I'm guessing it's the same as `--keep-empty`. -- Philip
Re: git send-email Cc with cruft not working as expected
On Wed, Aug 23, 2017 at 3:02 AM, Matthieu Moy wrote: >> On Tue, Aug 22, 2017 at 4:30 PM, Jacob Keller wrote: >>> Additionally I just discovered that the behavior here changes pretty >>> drastically if you have Email::Validate installed, now it splits the >>> address into multiple things: > > (I'm assuming you mean Email::Address, there's also Email::Valid but I > don't think it would modify the behavior) > No I actually definitely meant Email::Valid. I already had Mail::Address installed, and I then installed Email::Valid, and it changed behavior to split the cruft into multiple addresses. I don't actually know why or how it did this, but I'm certain it was presence of Email::Valid that did it. However, your first patch addresses the issue since you remove the cruft well before passing it into Email::Valid anyways. > Hmm, I think we reached the point where we should just stop using > Email::Address. I do agree, I don't think we should use Mail::Address. > > Patch series follows and should address both points. > > -- > Matthieu Moy > http://matthieu-moy.fr
Re: splitting off shell test framework
On 23 August 2017 at 16:47, Jeff King wrote: > On Wed, Aug 23, 2017 at 02:46:30PM +0100, Adam Spiers wrote: >> >> Done at least once already: >> >> >> >> http://comments.gmane.org/gmane.comp.version-control.git/201591 >> [...] >> >> but sadly since then gmane has shuffled off its mortal coil and I can't >> remember / find what this URL referred to. Please could someone >> point me at a working link? > > Try: > > https://public-inbox.org/git/?q=gmane:201591 > > Public-inbox uses message-ids as its primary key, but keeps a static > mapping of gmane articles to message-ids. But the corpus of gmane ids > isn't growing anymore, and public-inbox URLs can be trivially converted > to other systems which index on the message id[1]. Thanks a lot; that's super helpful! > [1] I actually keep a local archive and convert public-inbox URLs into > local requests that I view in mutt. Sounds like a neat trick - any scripts / config worth sharing?
Re: [PATCH 2/2] treewide: correct several "up-to-date" to "up to date"
These corrections all look great (English-language-wise). :) On Wed, Aug 23, 2017 at 10:49 AM, Martin Ågren wrote: > Follow the Oxford style, which says to use "up-to-date" before the noun, > but "up to date" after it. Don't change plumbing (specifically > send-pack.c, but transport.c (git push) also has the same string). > > This was produced by grepping for "up-to-date" and "up to date". It > turned out we only had to edit in one direction, removing the hyphens. > > Fix a typo in Documentation/git-diff-index.txt while we're there. > > Reported-by: Jeffrey Manian > Reported-by: STEVEN WHITE > Signed-off-by: Martin Ågren > --- > I figure "Already up-to-date." is "[X is|I am] already up to date." as > opposed to "Already [have] up-to-date [X].". > > Documentation/git-apply.txt | 4 ++-- > Documentation/git-cvsserver.txt | 2 +- > Documentation/git-diff-index.txt | 6 +++--- > Documentation/git-merge.txt | 2 +- > Documentation/git-rebase.txt | 2 +- > Documentation/git-rerere.txt | 2 +- > Documentation/git-rm.txt | 2 +- > Documentation/git-svn.txt | 2 +- > Documentation/git-update-index.txt| 2 +- > Documentation/gitcore-tutorial.txt| 8 > Documentation/githooks.txt| 2 +- > Documentation/gitrepository-layout.txt| 2 +- > Documentation/gittutorial.txt | 2 +- > Documentation/merge-options.txt | 2 +- > Documentation/technical/pack-protocol.txt | 2 +- > Documentation/technical/trivial-merge.txt | 4 ++-- > Documentation/user-manual.txt | 2 +- > t/t6040-tracking-info.sh | 4 ++-- > contrib/examples/git-merge.sh | 4 ++-- > contrib/examples/git-resolve.sh | 2 +- > contrib/subtree/t/t7900-subtree.sh| 2 +- > git-merge-octopus.sh | 2 +- > builtin/merge.c | 4 ++-- > merge-recursive.c | 2 +- > notes-merge.c | 2 +- > remote.c | 2 +- > unpack-trees.c| 2 +- > git-gui/po/README | 2 +- > git-p4.py | 2 +- > templates/hooks--pre-rebase.sample| 2 +- > 30 files changed, 40 insertions(+), 40 deletions(-) > > diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt > index 631cbd840..4ebc3d327 100644 > --- a/Documentation/git-apply.txt > +++ b/Documentation/git-apply.txt > @@ -66,7 +66,7 @@ OPTIONS > disables it is in effect), make sure the patch is > applicable to what the current index file records. If > the file to be patched in the working tree is not > - up-to-date, it is flagged as an error. This flag also > + up to date, it is flagged as an error. This flag also > causes the index file to be updated. > > --cached:: > @@ -259,7 +259,7 @@ treats these changes as follows. > If `--index` is specified (explicitly or implicitly), then the submodule > commits must match the index exactly for the patch to apply. If any > of the submodules are checked-out, then these check-outs are completely > -ignored, i.e., they are not required to be up-to-date or clean and they > +ignored, i.e., they are not required to be up to date or clean and they > are not updated. > > If `--index` is not specified, then the submodule commits in the patch > diff --git a/Documentation/git-cvsserver.txt b/Documentation/git-cvsserver.txt > index a336ae5f6..ba90066f1 100644 > --- a/Documentation/git-cvsserver.txt > +++ b/Documentation/git-cvsserver.txt > @@ -223,7 +223,7 @@ access method and requested operation. > That means that even if you offer only read access (e.g. by using > the pserver method), 'git-cvsserver' should have write access to > the database to work reliably (otherwise you need to make sure > -that the database is up-to-date any time 'git-cvsserver' is executed). > +that the database is up to date any time 'git-cvsserver' is executed). > > By default it uses SQLite databases in the Git directory, named > `gitcvs..sqlite`. Note that the SQLite backend creates > diff --git a/Documentation/git-diff-index.txt > b/Documentation/git-diff-index.txt > index a17150695..b38067771 100644 > --- a/Documentation/git-diff-index.txt > +++ b/Documentation/git-diff-index.txt > @@ -85,7 +85,7 @@ a 'git write-tree' + 'git diff-tree'. Thus that's the > default mode. > The non-cached version asks the question: > >show me the differences between HEAD and the currently checked out > - tree - index contents _and_ files that aren't up-to-date > + tree - index contents _and_ files that aren't up to date > > which is obviously a very useful question too, since that tells you what > you *could* commit. Again, the output matches the 'git diff-tree -r' > @@ -100,8 +100,8 @@ have not actually done a 'git update-index'
Re: Undocumented change in `git branch -M` behavior
On Wed, Aug 23, 2017 at 01:13:34PM -0700, Nish Aravamudan wrote: > Hello, > > Hopefully, I've got this right -- I noticed a change in behavior in git > with Ubuntu 17.10, which recently got 2.14.1. Specifically, that when in > an orphaned branch, -M ends up moving HEAD to the new branch name, > clobbering the working tree. As far as I know, from the manpages, > orphaned branches are still supported and should work? > > I think an example will demonstrate more than words (the following are > done in LXD containers, hence the root user): > > # git --version > git version 2.14.1 > # mkdir test && cd test && git init . > Initialized empty Git repository in /root/test/.git/ > # git checkout -b a > Switched to a new branch 'a' > # touch testfile && git add testfile && git commit -m 'initial commit' > [a (root-commit) 6061193] initial commit > Committer: root > 1 file changed, 0 insertions(+), 0 deletions(-) > create mode 100644 testfile > # git checkout --orphan master > Switched to a new branch 'master' > # git status > On branch master > > No commits yet > > Changes to be committed: > (use "git rm --cached ..." to unstage) > > new file: testfile > > # git reset --hard && git status > On branch master > > No commits yet > > nothing to commit (create/copy files and use "git add" to track) > # git branch -M a b > # git status > On branch b > Changes to be committed: > (use "git reset HEAD ..." to unstage) > > deleted:testfile > > This is very unexpected. I force-renamed a branch I wasn't currently > checked out to and now I'm checked out to it *and* I have staged file > removals (I think what is effectively happening is my current working > directory (empty) is being staged into the new branch, but I'm not > 100%). > > For comparision, on 17.04: > > # git --version > git version 2.11.0 > # mkdir test && cd test && git init . > Initialized empty Git repository in /root/test/.git/ > # git checkout -b a > Switched to a new branch 'a' > # touch testfile && git add testfile && git commit -m 'initial commit' > [a (root-commit) f8d0d53] initial commit > Committer: root > 1 file changed, 0 insertions(+), 0 deletions(-) > create mode 100644 testfile > # git checkout --orphan master > Switched to a new branch 'master' > # git status > On branch master > > No commits yet > > Changes to be committed: > (use "git rm --cached ..." to unstage) > > new file: testfile > > # git reset --hard && git status > On branch master > > No commits yet > > nothing to commit (create/copy files and use "git add" to track) > # git branch -M a b > # git status > On branch master > > Initial commit > > nothing to commit (create/copy files and use "git add" to track) > > This is what I expect to see, the branch rename has no effect on HEAD. > > I haven't yet bisected this (but I can if necessary). My initial > suspicion is > https://github.com/git/git/commit/70999e9ceca47e03b8900bfb310b2f804125811e#diff-d18f86ea14e2f1e5bff391b2e54438cb > where a comparison between the oldname of the branch and HEAD was > performed before attempting to move HEAD (so that HEAD followed to the > new branch name, I believe). That change was dropped, though and perhaps > the new check in replace_each_worktree_head_symref of > > strcmp(oldref, worktrees[i]->head_ref) > > does not work for orphaned branches? I am unfamiliar with all the > details of the git internals, so please correct me if I'm wrong! > > Thanks, > Nish > > -- > Nishanth Aravamudan > Ubuntu Server > Canonical Ltd Thanks for this report. I've bisected it down to fa099d232 (worktree.c: kill parse_ref() in favor of refs_resolve_ref_unsafe(), 2017-04-24) I've CC'ed Duy, who made that commit. Kevin
Re: [PATCH] Documentation: mention that `eol` can change the dirty status of paths
On Wed, Aug 23, 2017 at 05:17:41PM -0400, Ben Boeckel wrote: > When setting the `eol` attribute, paths can change their dirty status > without any change in the working directory. This can cause confusion > and should at least be mentioned with a remedy. > > Signed-off-by: Ben Boeckel > --- > Documentation/gitattributes.txt | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt > index c4f2be2..3044b71 100644 > --- a/Documentation/gitattributes.txt > +++ b/Documentation/gitattributes.txt > @@ -151,7 +151,11 @@ unspecified. > > This attribute sets a specific line-ending style to be used in the > working directory. It enables end-of-line conversion without any > -content checks, effectively setting the `text` attribute. > +content checks, effectively setting the `text` attribute. Note that > +setting this attribute on paths which are in the index with different > +line endings than the attribute indicates the paths to be considered > +dirty. Adding the path to the index again will normalize the line > +endings in the index. > There is one minor comment: The problem is when files had been commited with CRLF (regardless what your eol= attribute says) How about something like this : This attribute sets a specific line-ending style to be used in the working directory. It enables end-of-line conversion without any -content checks, effectively setting the `text` attribute. +content checks, effectively setting the `text` attribute. Note that +setting this attribute on paths which are in the index with CRLF +line endings makes the paths to be considered dirty. +Adding the path to the index again will normalize the line +endings in the index.
Re: [PATCH v3 2/4] imap-send: add wrapper to get server credentials if needed
Le 23/08/2017 à 22:13, Junio C Hamano a écrit :
> Nicolas Morey-Chaisemartin writes:
>
>> Signed-off-by: Nicolas Morey-Chaisemartin
>> ---
>> imap-send.c | 34 --
>> 1 file changed, 20 insertions(+), 14 deletions(-)
>>
>> diff --git a/imap-send.c b/imap-send.c
>> index 09f29ea95..448a4a0b3 100644
>> --- a/imap-send.c
>> +++ b/imap-send.c
>> @@ -926,6 +926,25 @@ static int auth_cram_md5(struct imap_store *ctx, struct
>> imap_cmd *cmd, const cha
>> return 0;
>> }
>>
>> +static void server_fill_credential(struct imap_server_conf *srvc, struct
>> credential *cred)
>> +{
>> +if (srvc->user && srvc->pass)
>> +return;
>> +
>> +cred->protocol = xstrdup(srvc->use_ssl ? "imaps" : "imap");
>> +cred->host = xstrdup(srvc->host);
>> +
>> +cred->username = xstrdup_or_null(srvc->user);
>> +cred->password = xstrdup_or_null(srvc->pass);
>> +
>> +credential_fill(cred);
>> +
>> +if (!srvc->user)
>> +srvc->user = xstrdup(cred->username);
>> +if (!srvc->pass)
>> +srvc->pass = xstrdup(cred->password);
>> +}
>> +
> This looks straight-forward code movement. The only thing that
> makes me wonder is if this is "server". The existing naming of the
> variables screams at me that this is not "server" but is "service".
I read srvc as server conf not service.
But I can change the name if you prefer
Nicolas
