Re: What's cooking in git.git (Aug 2018, #06; Wed, 29)
On 30 August 2018 21:41:35 GMT+05:30, Elijah Newren wrote: >That makes sense. I'm not sure I can concisely convey all the right >points, but here's a stab at rewording: > >Recent addition of "directory rename" heuristics to the >merge-recursive backend makes the command susceptible to false >positives and false negatives. In the context of "git am -3", which >does not know about surrounding unmodified paths and thus cannot >inform the merge machinery about the full trees involved, this risk is >particularly severe. As such, the heuristic is disabled for "git am >-3" to keep the machinery "more stupid but predictable". FWIW, this sounds better to me than the original. -- Sivaraam Sent from my Android device with K-9 Mail. Please excuse my brevity.
[PATCH 0/2] rebase --autosquash: handle manual "final fixups"
'tis bug fix season! I admit, though, that this bug might be a bit too off the trodden path to merit fast-tracking into v2.19.0. While pairing with Jameson Miller to rebase Git for Windows to v2.19.0-rc1, we had to fix a couple of commits which had somehow lost their proper authorship (probably due to long fixed bugs in the interactive rebase). We did so by using empty squash! commits as reminders, so that we could interrupt the rebase by deleting the squash message, amend the commit appropriately, and then continue. This exposed an (admittedly obscure) bug in the interactive rebase: when the last fixup or squash of a fixup/squash chain is aborted, and then the HEAD commit is amended, the rebase would not forget about the fixup/squash chain. It would hold onto the information about the current fixup count and the intermediate commit message. And upon processing another fixup/squash chain, that information would be reused! This patch pair first introduces the test case to confirm the breakage, and then fixes it in the minimal way. Johannes Schindelin (2): rebase -i --autosquash: demonstrate a problem skipping the last squash rebase -i: be careful to wrap up fixup/squash chains sequencer.c | 17 ++--- t/t3415-rebase-autosquash.sh | 19 +++ 2 files changed, 33 insertions(+), 3 deletions(-) base-commit: 2f743933341f27603550fbf383a34dfcfd38 Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-30%2Fdscho%2Frebase-abort-last-squash-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-30/dscho/rebase-abort-last-squash-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/30 -- gitgitgadget
[PATCH 2/2] rebase -i: be careful to wrap up fixup/squash chains
From: Johannes Schindelin
When an interactive rebase was stopped at the end of a fixup/squash
chain, the user might have edited the commit manually before continuing
(with either `git rebase --skip` or `git rebase --continue`, it does not
really matter which).
We need to be very careful to wrap up the fixup/squash chain also in
this scenario: otherwise the next fixup/squash chain would try to pick
up where the previous one was left.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 17 ++---
t/t3415-rebase-autosquash.sh | 2 +-
2 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 84bf598c3e..ac5c805c14 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3578,9 +3578,20 @@ static int commit_staged_changes(struct replay_opts
*opts,
* the commit message and if there was a squash, let the user
* edit it.
*/
- if (is_clean && !oidcmp(, _amend) &&
- opts->current_fixup_count > 0 &&
- file_exists(rebase_path_stopped_sha())) {
+ if (!is_clean || !opts->current_fixup_count)
+ ; /* this is not the final fixup */
+ else if (oidcmp(, _amend) ||
+!file_exists(rebase_path_stopped_sha())) {
+ /* was a final fixup or squash done manually? */
+ if (!is_fixup(peek_command(todo_list, 0))) {
+ unlink(rebase_path_fixup_msg());
+ unlink(rebase_path_squash_msg());
+ unlink(rebase_path_current_fixups());
+ strbuf_reset(>current_fixups);
+ opts->current_fixup_count = 0;
+ }
+ } else {
+ /* we are in a fixup/squash chain */
const char *p = opts->current_fixups.buf;
int len = opts->current_fixups.len;
diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 7d5ea340b3..13f5688135 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -330,7 +330,7 @@ test_expect_success 'wrapped original subject' '
test $base = $parent
'
-test_expect_failure 'abort last squash' '
+test_expect_success 'abort last squash' '
test_when_finished "test_might_fail git rebase --abort" &&
test_when_finished "git checkout master" &&
--
gitgitgadget
[PATCH 1/2] rebase -i --autosquash: demonstrate a problem skipping the last squash
From: Johannes Schindelin The `git commit --squash` command can be used not only to amend commit messages and changes, but also to record notes for an upcoming rebase. For example, when the author information of a given commit is incorrect, a user might call `git commit --allow-empty -m "Fix author" --squash `, to remind them to fix that during the rebase. When the editor would pop up, the user would simply delete the commit message to abort the rebase at this stage, fix the author information, and continue with `git rebase --skip`. (This is a real-world example from the rebase of Git for Windows onto v2.19.0-rc1.) However, there is a bug in `git rebase` that will cause the squash message *not* to be forgotten in this case. It will therefore be reused in the next fixup/squash chain (if any). This patch adds a test case to demonstrate this breakage. Signed-off-by: Johannes Schindelin --- t/t3415-rebase-autosquash.sh | 19 +++ 1 file changed, 19 insertions(+) diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh index e364c12622..7d5ea340b3 100755 --- a/t/t3415-rebase-autosquash.sh +++ b/t/t3415-rebase-autosquash.sh @@ -330,4 +330,23 @@ test_expect_success 'wrapped original subject' ' test $base = $parent ' +test_expect_failure 'abort last squash' ' + test_when_finished "test_might_fail git rebase --abort" && + test_when_finished "git checkout master" && + + git checkout -b some-squashes && + git commit --allow-empty -m first && + git commit --allow-empty --squash HEAD && + git commit --allow-empty -m second && + git commit --allow-empty --squash HEAD && + + test_must_fail git -c core.editor="grep -q ^pick" \ + rebase -ki --autosquash HEAD~4 && + : do not finish the squash, but resolve it manually && + git commit --allow-empty --amend -m edited-first && + git rebase --skip && + git show >actual && + ! grep first actual +' + test_done -- gitgitgadget
Re: [PATCH 5/6] pack-bitmap: save "have" bitmap from walk
On Fri, Aug 31, 2018 at 05:23:17PM +0200, Ævar Arnfjörð Bjarmason wrote:
> On Tue, Aug 21 2018, Jeff King wrote:
>
> > +int bitmap_has_sha1_in_uninteresting(struct bitmap_index *bitmap_git,
> > +const unsigned char *sha1)
> > +{
> > + int pos;
> > +
> > + if (!bitmap_git)
> > + return 0; /* no bitmap loaded */
> > + if (!bitmap_git->result)
> > + BUG("failed to perform bitmap walk before querying");
>
> Some part of what calls this completely breaks pushing from the "next"
> branch when you have local bitmaps (we *really* should have some tests
> for this...).
Yikes, thanks for reporting. I agree we need better tests here.
This assertion is totally bogus, as traverse_bitmap_commit_list() will
actually free (and NULL) the result field! This is a holdover from the
original bitmap code, where bitmap_git was a static, and this is how you
might prepare a second walk (though AFAIK only ever do one walk per
process). These days prepare_bitmap_walk() actually returns a fresh
bitmap_index struct.
So there's no way to know if traverse_bitmap_commit_list() was called.
But as it turns out, we don't care. We want to know if
"bitmap_git->have" was set up, which is done in prepare_bitmap_walk().
So there's no way[1] to get a bitmap_git that hasn't been properly set
up. This BUG() check can just go away.
I'll prepare a patch and some tests later tonight.
-Peff
[1] Actually, there is also prepare_bitmap_git(), but it is not really
for general use by callers. It should be made static, or better yet,
I suspect it can be folded into its callers.
Re: [PATCH v5 2/9] push tests: make use of unused $1 in test description
On Fri, Aug 31 2018, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason writes:
>
>> Fix up a logic error in 380efb65df ("push tests: assert re-pushing
>> annotated tags", 2018-07-31), where the $tag_type_description variable
>> was assigned to but never used, unlike in the subsequently added
>> companion test for fetches in 2d216a7ef6 ("fetch tests: add a test for
>> clobbering tag behavior", 2018-04-29).
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason
>> ---
>> t/t5516-fetch-push.sh | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
>> index 539c25aada..62d5059f92 100755
>> --- a/t/t5516-fetch-push.sh
>> +++ b/t/t5516-fetch-push.sh
>> @@ -969,7 +969,7 @@ test_force_push_tag () {
>> tag_type_description=$1
>> tag_args=$2
>>
>> -test_expect_success 'force pushing required to update lightweight tag' "
>> +test_expect_success 'force pushing required to update
>> $tag_type_description' "
>
> Of course, $1 needs to be inside "dq-pair" for $tag_type_description
> to be substituted ;-) So I'll tweak it while queuing.
D'oh! I knew I'd miss something. Hopefully this was the only thing.
> All the other ones in this series looked sensible to me. Will
> replace.
Thanks!
Re: [PATCH 0/9] worktree: fix bugs and broaden --force applicability
On 31/08/18 01:54, Jeff King wrote:
> On Fri, Aug 31, 2018 at 12:49:39AM +0100, Ramsay Jones wrote:
>
>> On 30/08/18 21:14, Junio C Hamano wrote:
>>> Jeff King writes:
>>>
I suppose so. I don't think I've _ever_ used distclean, and I only
rarely use "clean" (a testament to our Makefile's efforts to accurately
track dependencies). I'd usually use "git clean" when I want something
pristine (because I don't want to trust the Makefile at all).
>>>
>>> I do not trust "git clean" all that much, and pre-cleaning with
>>> "make distclean" and then running "git clean -x" has become my bad
>>> habit. I jump around quite a bit during the day, which would end up
>>> littering the working tree with *.o files that are only known to one
>>> but not both of {maint,pu}/Makefile's distclean rules. I even do
>>> "for i in pu maint master next; do git checkout $i; make distclean; done"
>>> sometimes before running "git clean -x" ;-)
>>>
>>
>> 'git clean -x' always removes _way_ more than I want it
>> to - in particular, I lost my config.mak more than once.
>
> Heh. I have done that, too, but fortunately mine is a symlink to a copy
> that is held in a git repository. ;)
:-D
Now, why didn't I think of that! ;-)
ATB,
Ramsay Jones
Re: [PATCH 2/5] t5303: test some corrupt deltas
Jeff King writes: > - make it clear to other observers that there's at least one person > who hopes this is not the norm for this list Make it at least two by counting me ;-) > - make a general reminder that collaborating on the Internet is hard, > but it's worth spending the extra effort to consider how comments > will be taken by the intended receiver, as well as others on the > list Well, in this particular case both of us know exactly how the sender wants the message to be taken by the other side. At least I do.
Re: [PATCH 2/5] t5303: test some corrupt deltas
On Fri, Aug 31, 2018 at 11:14:20PM +0200, Johannes Schindelin wrote: > On Fri, 31 Aug 2018, Junio C Hamano wrote: > [...] > > So instead of typing 3 lines, you can just say "yes we use echo that > > emulates Unix". > > You could just express your gratitude that I do more than just answer a > question, and impart more knowledge that will help you be a better > maintainer. It is my observation as a third party that there has been a lot of bickering between you two lately (here and in other threads). Can you both please try to make sure your comments are truly constructive? I don't want to get into details of who I think has been unreasonable when, or lay any kind of blame. I'm just concerned that exchanges like this one worsen the general tone of the mailing list, and I'd like to: - make it clear to other observers that there's at least one person who hopes this is not the norm for this list - make a general reminder that collaborating on the Internet is hard, but it's worth spending the extra effort to consider how comments will be taken by the intended receiver, as well as others on the list You're free to disagree, of course. And I'm sorry for being somewhat vague. I really want to avoid opening up a specific flamewar, and just make a general call for people to remember to be friendly. If vger allowed HTML mail, I would link to a picture of a unicorn running across a rainbow. -Peff
Re: [PATCH 2/5] t5303: test some corrupt deltas
Jeff King writes: >> So instead of typing 3 lines, you can just say "yes we use echo that >> emulates Unix". > > I actually found Dscho's response much more informative than a simple > yes/no. > > At any rate, it sounds like we are probably OK with echo, but I think it > is still worth doing the defensive patch-on-top that I posted. Actually, Dscho's footnote was much more informative than the "echo is from UNIX, dammit" that didn't convey anything useful---yes, we all know that echo is from UNIX, but what does he do to it? Does he want it to be like UNIX, or want to tweak it to suit Windows taste? Reading the footnote again, from there, you can read that 'echo' may currently behave like UNIX, but he guarantees that it can change any time without notice; in his mind, 'echo' in the ideal world in the fiture on Windows is to use CRLF. So we must protect the test from his whim by using printf.
Re: Possible bug: identical lines added/removed in git diff
Hi, On Thu, 30 Aug 2018, Stefan Beller wrote: > On Thu, Aug 30, 2018 at 12:20 PM Jeff King wrote: > > > > [...] Myers does not promise to find the absolute minimal diff. [...] > > The `Myers` (our default) diff algorithm is really the Myers algorithm + > a heuristic that cuts off the long tail when it is very costly to compute > the minimal diff. IIRC Myers promises minimal diffs only for -U0. As soon as you add context, Myers might in fact end up with a suboptimal diff, even without that heuristic. Ciao, Dscho
Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on "make clean"
On Fri, Aug 31, 2018 at 4:07 PM Jeff King wrote: > On Fri, Aug 31, 2018 at 02:33:18AM -0400, Eric Sunshine wrote: > > diff --git a/Documentation/Makefile b/Documentation/Makefile > > @@ -332,6 +332,7 @@ clean: > > $(RM) manpage-base-url.xsl > > + '$(SHELL_PATH_SQ)' ./doc-diff --clean > > This spelling took me by surprise. The doc-diff script itself specifies > /bin/sh, and we do not build it, so the #! line is never replaced. [...] > > I don't think the script does anything complicated that would choke a > lesser /bin/sh. But it doesn't hurt to be defensive, since this bit of > the Makefile will be run for everyone, whether they care about doc-diff > or not. > > So that all makes sense. I initially wrote this to suggest that we call > out this subtlety in the commit message. But I see this is based on > existing instances from ee7ec2f9de (documentation: Makefile accounts for > SHELL_PATH setting, 2009-03-22). So maybe I am just showing my > ignorance. ;) Correct. I was concerned that invoking it simply as "./doc-diff --clean" could be problematic, so, knowing that the Makefile invoked other scripts in Documentation/, I mirrored their invocation. If it didn't follow existing practice of invoking the command with $(SHELL_PATH_SQ), then that would merit mention in the commit message, but as it is, the commit message is probably fine. Thanks for the review.
Re: [PATCH 2/3] doc-diff: add --clean mode to remove temporary working gunk
On Fri, Aug 31, 2018 at 4:01 PM Jeff King wrote: > On Fri, Aug 31, 2018 at 02:33:17AM -0400, Eric Sunshine wrote: > > OPTIONS_SPEC="\ > > doc-diff [options] [-- ] > > +doc-diff (-c|--clean) > > -- > > j=n parallel argument to pass to make > > fforce rebuild; do not rely on cached results > > +c,clean cleanup temporary working files > > " > > This will cause parseopt to normalize "--clean" to "-c" (along with > "--cle", etc). Good to know. The documentation for git-sh-setup didn't talk about that at all, and while git-rev-parse documentation says that it "normalizes" options, that word didn't really convey this specific meaning to me, so I missed it. > > while test $# -gt 0 > > do > > case "$1" in > > -j) > > parallel=$2; shift ;; > > + -c|--clean) > > + clean=t ;; > > So this part can just test for "-c". AFAICT this is how "rev-parse > --parseopt" has always worked, though the documentation is quite > unclear. Other scripts seem to also use these redundant long options. > I'm not opposed to including it as a defensive measure (or simply an > annotation for the reader). I'm fine leaving it as-is too since it seems that every other client of git-sh-setup does the same (and to save a re-roll).
Re: [PATCH 2/5] t5303: test some corrupt deltas
Hi Junio, On Fri, 31 Aug 2018, Junio C Hamano wrote: > Johannes Schindelin writes: > > >> Would "echo base >base" give us 5-byte long base even on Windows? > > > > Please note that Unix shell scripting is a foreign thing on Windows. As > > such, there is not really any "native" shell we can use [*1*], and > > Yeah, I know that; otherwise I wouldn't have asked. Because ... > > > therefore we use MSYS2's Bash which outputs Unix line endings. > > ... I didn't know what MSYS folks chose, and/or if you have chosen > to tweak their choice, and/or if you switched to somebody else's shell > (e.g. busybox) and/or you chose to tweak what they do out of the box, > it was worth asking and getting yes/no question. You do not have to > tell me why I should be asking. > > So instead of typing 3 lines, you can just say "yes we use echo that > emulates Unix". > > Thanks. You could just express your gratitude that I do more than just answer a question, and impart more knowledge that will help you be a better maintainer. Ciao, Dscho
Re: [PATCH v5 2/9] push tests: make use of unused $1 in test description
Ævar Arnfjörð Bjarmason writes:
> Fix up a logic error in 380efb65df ("push tests: assert re-pushing
> annotated tags", 2018-07-31), where the $tag_type_description variable
> was assigned to but never used, unlike in the subsequently added
> companion test for fetches in 2d216a7ef6 ("fetch tests: add a test for
> clobbering tag behavior", 2018-04-29).
>
> Signed-off-by: Ævar Arnfjörð Bjarmason
> ---
> t/t5516-fetch-push.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 539c25aada..62d5059f92 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -969,7 +969,7 @@ test_force_push_tag () {
> tag_type_description=$1
> tag_args=$2
>
> - test_expect_success 'force pushing required to update lightweight tag' "
> + test_expect_success 'force pushing required to update
> $tag_type_description' "
Of course, $1 needs to be inside "dq-pair" for $tag_type_description
to be substituted ;-) So I'll tweak it while queuing.
All the other ones in this series looked sensible to me. Will
replace.
Thanks.
> mk_test testrepo heads/master &&
> mk_child testrepo child1 &&
> mk_child testrepo child2 &&
Re: [PATCH] config.mak.uname: resolve FreeBSD iconv-related compilation warning
On Fri, Aug 31, 2018 at 1:00 PM Jonathan Nieder wrote: > > From: Jonathan Nieder > > OLD_ICONV has long been needed by FreeBSD so config.mak.uname defines > > it unconditionally. However, recent versions do not need it, and its > > presence results in compilation warnings. Resolve this issue by defining > > OLD_ICONV only for older FreeBSD versions. > > I think it makes sense for you to take credit for this one. You > noticed the original problem, tested on FreeBSD, wrote the > explanation, and figured out the firstword hackery. All I did was to > say "somebody should fix this" and run "git log -S" a few times. [...] I'm fine going either way with authorship, and I can re-roll with that minor change (if Junio isn't interested in tweaking it while queueing).
Re: [PATCH v2 1/1] builtin rebase: prepare for builtin rebase -i
On Fri, Aug 31, 2018 at 10:38:44PM +0200, Johannes Schindelin wrote:
> > > Is there any reason why you avoid using `git rebase -ir` here? This should
> > > be so much easier via
> > >
> > > git checkout pk/rebase-i-in-c-6-final
> > > git rebase -ir js/rebase-in-c-5.5-work-with-rebase-i-in-c^
> > >
> > > and then inserting this at the appropriate position, followed by the `git
> > > range-diff @{-1}...`:
> > >
> > > git am -s mbox
> > > git update-ref js/rebase-in-c-5.5-work-with-rebase-i-in-c HEAD
> >
> > Related discussion, including a fantasy tangent by me (downthread):
> >
> >
> > https://public-inbox.org/git/20180727080807.ga11...@sigill.intra.peff.net/#t
>
> I have no idea what you meant there...
I thought you were asking why Junio does not just use "git am" from
inside "git rebase". I asked the same thing recently, and the answer is
because he is afraid of how the two interact. I dug a little into it
(the fantasy part is that I laid out a dream for how operations like
this could safely stack).
-Peff
Re: [PATCH v2 1/1] builtin rebase: prepare for builtin rebase -i
Hi Peff,
On Thu, 30 Aug 2018, Jeff King wrote:
> On Thu, Aug 30, 2018 at 01:03:41PM +0200, Johannes Schindelin wrote:
>
> > > Will replace by doing:
> > >
> > > $ git checkout js/rebase-in-c-5.5-work-with-rebase-i-in-c
> > > $ git checkout HEAD^
> > > $ git am -s mbox
> > > $ git range-diff @{-1}...
> > > $ git checkout -B @{-1}
> > >
> > > $ git checkout pk/rebase-i-in-c-6-final
> > > $ git rebase --onto js/rebase-in-c-5.5-work-with-rebase-i-in-c \
> > > js/rebase-in-c-5.5-work-with-rebase-i-in-c@{1} HEAD^0
> > > $ git range-diff @{-1}...
> > > $ git checkout -B @{-1}
> > >
> > > to update the two topics and then rebuilding the integration
> > > branches the usual way. I also need to replace the "other" topic
> > > used in this topic, so the actual procedure would be a bit more
> > > involved than the above, though.
> >
> > Is there any reason why you avoid using `git rebase -ir` here? This should
> > be so much easier via
> >
> > git checkout pk/rebase-i-in-c-6-final
> > git rebase -ir js/rebase-in-c-5.5-work-with-rebase-i-in-c^
> >
> > and then inserting this at the appropriate position, followed by the `git
> > range-diff @{-1}...`:
> >
> > git am -s mbox
> > git update-ref js/rebase-in-c-5.5-work-with-rebase-i-in-c HEAD
>
> Related discussion, including a fantasy tangent by me (downthread):
>
> https://public-inbox.org/git/20180727080807.ga11...@sigill.intra.peff.net/#t
I have no idea what you meant there...
Ciao,
Dscho
[PATCH v5 8/9] fetch: document local ref updates with/without --force
Refer to the new git-push(1) documentation about when ref updates are
and aren't allowed with and without --force, noting how "git-fetch"
differs from the behavior of "git-push".
Perhaps it would be better to split this all out into a new
gitrefspecs(7) man page, or present this information using tables.
In lieu of that, this is accurate, and fixes a big omission in the
existing refspec docs.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
Documentation/fetch-options.txt| 15 +-
Documentation/pull-fetch-param.txt | 32 +-
2 files changed, 37 insertions(+), 10 deletions(-)
diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 8bc36af4b1..fa0a3151b3 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -68,11 +68,16 @@ endif::git-pull[]
-f::
--force::
- When 'git fetch' is used with `:`
- refspec, it refuses to update the local branch
- `` unless the remote branch `` it
- fetches is a descendant of ``. This option
- overrides that check.
+ When 'git fetch' is used with `:` refspec it may
+ refuse to update the local branch as discussed
+ifdef::git-pull[]
+ in the `` part of the linkgit:git-fetch[1]
+ documentation.
+endif::git-pull[]
+ifndef::git-pull[]
+ in the `` part below.
+endif::git-pull[]
+ This option overrides that check.
-k::
--keep::
diff --git a/Documentation/pull-fetch-param.txt
b/Documentation/pull-fetch-param.txt
index f1fb08dc68..ab9617ad01 100644
--- a/Documentation/pull-fetch-param.txt
+++ b/Documentation/pull-fetch-param.txt
@@ -33,11 +33,33 @@ name.
it requests fetching everything up to the given tag.
+
The remote ref that matches
-is fetched, and if is not an empty string, the local
-ref that matches it is fast-forwarded using .
-If the optional plus `+` is used, the local ref
-is updated even if it does not result in a fast-forward
-update.
+is fetched, and if is not an empty string, an attempt
+is made to update the local ref that matches it.
++
+Whether that update is allowed without `--force` depends on the ref
+namespace it's being fetched to, the type of object being fetched, and
+whether the update is considered to be a fast-forward. Generally, the
+same rules apply for fetching as when pushing, see the `...`
+section of linkgit:git-push[1] for what those are. Exceptions to those
+rules particular to 'git fetch' are noted below.
++
+Unlike when pushing with linkgit:git-push[1], any updates to
+`refs/tags/*` will be accepted without `+` in the refspec (or
+`--force`). The receiving promiscuously considers all tag updates from
+a remote to be forced fetches.
++
+Unlike when pushing with linkgit:git-push[1], any updates outside of
+`refs/{tags,heads}/*` will be accepted without `+` in the refspec (or
+`--force`), whether that's swapping e.g. a tree object for a blob, or
+a commit for another commit that's doesn't have the previous commit as
+an ancestor etc.
++
+As with pushing with linkgit:git-push[1], all of the rules described
+above about what's not allowed as an update can be overridden by
+adding an the optional leading `+` to a refspec (or using `--force`
+command line option). The only exception to this is that no amount of
+forcing will make the `refs/heads/*` namespace accept a non-commit
+object.
+
[NOTE]
When the remote branch you want to fetch is known to
--
2.19.0.rc1.350.ge57e33dbd1
[PATCH v5 9/9] fetch: stop clobbering existing tags without --force
Change "fetch" to treat "+" in refspecs (aka --force) to mean we
should clobber a local tag of the same name.
This changes the long-standing behavior of "fetch" added in
853a3697dc ("[PATCH] Multi-head fetch.", 2005-08-20). Before this
change, all tag fetches effectively had --force enabled. See the
git-fetch-script code in fast_forward_local() with the comment:
> Tags need not be pointing at commits so there is no way to
> guarantee "fast-forward" anyway.
That commit and the rest of the history of "fetch" shows that the
"+" (--force) part of refpecs was only conceived for branch updates,
while tags have accepted any changes from upstream unconditionally and
clobbered the local tag object. Changing this behavior has been
discussed as early as 2011[1].
The current behavior doesn't make sense to me, it easily results in
local tags accidentally being clobbered. We could namespace our tags
per-remote and not locally populate refs/tags/*, but as with my
97716d217c ("fetch: add a --prune-tags option and fetch.pruneTags
config", 2018-02-09) it's easier to work around the current
implementation than to fix the root cause.
So this change implements suggestion #1 from Jeff's 2011 E-Mail[1],
"fetch" now only clobbers the tag if either "+" is provided as part of
the refspec, or if "--force" is provided on the command-line.
This also makes it nicely symmetrical with how "tag" itself works when
creating tags. I.e. we refuse to clobber any existing tags unless
"--force" is supplied. Now we can refuse all such clobbering, whether
it would happen by clobbering a local tag with "tag", or by fetching
it from the remote with "fetch".
Ref updates outside refs/{tags,heads/* are still still not symmetrical
with how "git push" works, as discussed in the recently changed
pull-fetch-param.txt documentation. This change brings the two
divergent behaviors more into line with one another. I don't think
there's any reason "fetch" couldn't fully converge with the behavior
used by "push", but that's a topic for another change.
One of the tests added in 31b808a032 ("clone --single: limit the fetch
refspec to fetched branch", 2012-09-20) is being changed to use
--force where a clone would clobber a tag. This changes nothing about
the existing behavior of the test.
1. https://public-inbox.org/git/2023221658.ga22...@sigill.intra.peff.net/
Signed-off-by: Ævar Arnfjörð Bjarmason
---
Documentation/pull-fetch-param.txt | 15 +++
builtin/fetch.c| 18 --
t/t5516-fetch-push.sh | 5 +++--
t/t5612-clone-refspec.sh | 4 ++--
4 files changed, 28 insertions(+), 14 deletions(-)
diff --git a/Documentation/pull-fetch-param.txt
b/Documentation/pull-fetch-param.txt
index ab9617ad01..293c6b967d 100644
--- a/Documentation/pull-fetch-param.txt
+++ b/Documentation/pull-fetch-param.txt
@@ -43,10 +43,13 @@ same rules apply for fetching as when pushing, see the
`...`
section of linkgit:git-push[1] for what those are. Exceptions to those
rules particular to 'git fetch' are noted below.
+
-Unlike when pushing with linkgit:git-push[1], any updates to
-`refs/tags/*` will be accepted without `+` in the refspec (or
-`--force`). The receiving promiscuously considers all tag updates from
-a remote to be forced fetches.
+Until Git version 2.20, and unlike when pushing with
+linkgit:git-push[1], any updates to `refs/tags/*` would be accepted
+without `+` in the refspec (or `--force`). The receiving promiscuously
+considered all tag updates from a remote to be forced fetches. Since
+Git version 2.20, fetching to update `refs/tags/*` work the same way
+as when pushing. I.e. any updates will be rejected without `+` in the
+refspec (or `--force`).
+
Unlike when pushing with linkgit:git-push[1], any updates outside of
`refs/{tags,heads}/*` will be accepted without `+` in the refspec (or
@@ -54,6 +57,10 @@ Unlike when pushing with linkgit:git-push[1], any updates
outside of
a commit for another commit that's doesn't have the previous commit as
an ancestor etc.
+
+Unlike when pushing with linkgit:git-push[1], there is no
+configuration which'll amend these rules, and nothing like a
+`pre-fetch` hook analogous to the `pre-receive` hook.
++
As with pushing with linkgit:git-push[1], all of the rules described
above about what's not allowed as an update can be overridden by
adding an the optional leading `+` to a refspec (or using `--force`
diff --git a/builtin/fetch.c b/builtin/fetch.c
index b0706b3803..ed4ed9d8c4 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -667,12 +667,18 @@ static int update_local_ref(struct ref *ref,
if (!is_null_oid(>old_oid) &&
starts_with(ref->name, "refs/tags/")) {
- int r;
- r = s_update_ref("updating tag", ref, 0);
- format_display(display, r ? '!' : 't', _("[tag update]"),
- r ? _("unable to update local ref") : NULL,
-
[PATCH v5 2/9] push tests: make use of unused $1 in test description
Fix up a logic error in 380efb65df ("push tests: assert re-pushing
annotated tags", 2018-07-31), where the $tag_type_description variable
was assigned to but never used, unlike in the subsequently added
companion test for fetches in 2d216a7ef6 ("fetch tests: add a test for
clobbering tag behavior", 2018-04-29).
Signed-off-by: Ævar Arnfjörð Bjarmason
---
t/t5516-fetch-push.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 539c25aada..62d5059f92 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -969,7 +969,7 @@ test_force_push_tag () {
tag_type_description=$1
tag_args=$2
- test_expect_success 'force pushing required to update lightweight tag' "
+ test_expect_success 'force pushing required to update
$tag_type_description' "
mk_test testrepo heads/master &&
mk_child testrepo child1 &&
mk_child testrepo child2 &&
--
2.19.0.rc1.350.ge57e33dbd1
[PATCH v5 3/9] push tests: use spaces in interpolated string
The quoted -m'msg' option would mean the same as -mmsg when passed
through the test_force_push_tag helper. Let's instead use a string
with spaces in it, to have a working example in case we need to pass
other whitespace-delimited arguments to git-tag.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
t/t5516-fetch-push.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 62d5059f92..8b67f08265 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1009,7 +1009,7 @@ test_force_push_tag () {
}
test_force_push_tag "lightweight tag" "-f"
-test_force_push_tag "annotated tag" "-f -a -m'msg'"
+test_force_push_tag "annotated tag" "-f -a -m'tag message'"
test_expect_success 'push --porcelain' '
mk_empty testrepo &&
--
2.19.0.rc1.350.ge57e33dbd1
[PATCH v5 4/9] fetch tests: add a test for clobbering tag behavior
The test suite only incidentally (and unintentionally) tested for the
current behavior of eager tag clobbering on "fetch". This is a
followup to 380efb65df ("push tests: assert re-pushing annotated
tags", 2018-07-31) which tests for it explicitly.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
t/t5516-fetch-push.sh | 24
1 file changed, 24 insertions(+)
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 8b67f08265..7f3d4c4965 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1011,6 +1011,30 @@ test_force_push_tag () {
test_force_push_tag "lightweight tag" "-f"
test_force_push_tag "annotated tag" "-f -a -m'tag message'"
+test_force_fetch_tag () {
+ tag_type_description=$1
+ tag_args=$2
+
+ test_expect_success "fetch will clobber an existing
$tag_type_description" "
+ mk_test testrepo heads/master &&
+ mk_child testrepo child1 &&
+ mk_child testrepo child2 &&
+ (
+ cd testrepo &&
+ git tag testTag &&
+ git -C ../child1 fetch origin tag testTag &&
+ >file1 &&
+ git add file1 &&
+ git commit -m 'file1' &&
+ git tag $tag_args testTag &&
+ git -C ../child1 fetch origin tag testTag
+ )
+ "
+}
+
+test_force_fetch_tag "lightweight tag" "-f"
+test_force_fetch_tag "annotated tag" "-f -a -m'tag message'"
+
test_expect_success 'push --porcelain' '
mk_empty testrepo &&
echo >.git/foo "To testrepo" &&
--
2.19.0.rc1.350.ge57e33dbd1
[PATCH v5 5/9] push doc: remove confusing mention of remote merger
Saying that "git push :" won't push a merger of
and to is clear from the rest of the context here,
so mentioning it is redundant, furthermore the mention of "EXAMPLES
below" isn't specific or useful.
This phrase was originally added in 149f6ddfb3 ("Docs: Expand
explanation of the use of + in git push refspecs.", 2009-02-19), as
can be seen in that change the point of the example being cited was to
show that force pushing can leave unreferenced commits on the
remote. It's enough that we explain that in its own section, it
doesn't need to be mentioned here.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
Documentation/git-push.txt | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 55277a9781..83e499ee97 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -78,8 +78,7 @@ on the remote side. By default this is only allowed if
is not
a tag (annotated or lightweight), and then only if it can fast-forward
. By having the optional leading `+`, you can tell Git to update
the ref even if it is not allowed by default (e.g., it is not a
-fast-forward.) This does *not* attempt to merge into . See
-EXAMPLES below for details.
+fast-forward.).
+
`tag ` means the same as `refs/tags/:refs/tags/`.
+
--
2.19.0.rc1.350.ge57e33dbd1
[PATCH v5 0/9] git fetch" should not clobber existing tags without --force
Addresses Junio's comments to v4, and I had a few fixes of my own. I
don't know if this range-diff is more or less readble than just
re-reading it, but here goes:
1: d05fd561f3 = 1: d05fd561f3 fetch: change "branch" to "reference" in
--force -h output
-: -- > 2: 28275baca2 push tests: make use of unused $1 in test
description
2: 013ecd83b3 ! 3: 834501afdc push tests: correct quoting in interpolated
string
@@ -1,24 +1,11 @@
Author: Ævar Arnfjörð Bjarmason
-push tests: correct quoting in interpolated string
+push tests: use spaces in interpolated string
-The quoted -m'msg' option is passed as a string to another function,
-where due to interpolation it'll end up meaning the same as if we did
-just did -m'msg' here.
-
-In [1] this was pointed out to me, but in submitting [2] the patches I
-missed this (since it was feedback on another patch I was holding
-off), so this logic error landed in 380efb65df ("push tests: assert
-re-pushing annotated tags", 2018-07-31).
-
-Let's just remove the quotes, and use a string that doesn't need to be
-quoted (-mtag.message is a bit less confusing than -mmsg). I could try
-to chase after getting the quoting right here with multiple
-backslashes, but I don't think it's worth it, and it makes things much
-less readable.
-
-1.
https://public-inbox.org/git/xmqq4lgfcn5a@gitster-ct.c.googlers.com/
-2.
https://public-inbox.org/git/20180813192249.27585-1-ava...@gmail.com/
+The quoted -m'msg' option would mean the same as -mmsg when passed
+through the test_force_push_tag helper. Let's instead use a string
+with spaces in it, to have a working example in case we need to pass
+other whitespace-delimited arguments to git-tag.
Signed-off-by: Ævar Arnfjörð Bjarmason
@@ -30,7 +17,7 @@
test_force_push_tag "lightweight tag" "-f"
-test_force_push_tag "annotated tag" "-f -a -m'msg'"
-+test_force_push_tag "annotated tag" "-f -a -mtag.message"
++test_force_push_tag "annotated tag" "-f -a -m'tag message'"
test_expect_success 'push --porcelain' '
mk_empty testrepo &&
3: 2d216a7ef6 ! 4: 5f85542bb2 fetch tests: add a test for clobbering tag
behavior
@@ -14,7 +14,7 @@
+++ b/t/t5516-fetch-push.sh
@@
test_force_push_tag "lightweight tag" "-f"
- test_force_push_tag "annotated tag" "-f -a -mtag.message"
+ test_force_push_tag "annotated tag" "-f -a -m'tag message'"
+test_force_fetch_tag () {
+ tag_type_description=$1
@@ -38,7 +38,7 @@
+}
+
+test_force_fetch_tag "lightweight tag" "-f"
-+test_force_fetch_tag "annotated tag" "-f -a -mtag.message"
++test_force_fetch_tag "annotated tag" "-f -a -m'tag message'"
+
test_expect_success 'push --porcelain' '
mk_empty testrepo &&
-: -- > 5: 6906d5a84d push doc: remove confusing mention of remote
merger
-: -- > 6: a16a9c2d7f push doc: move mention of "tag " later
in the prose
4: b751e80b00 ! 7: 9f8785e01a push doc: correct lies about how push
refspecs work
@@ -38,18 +38,20 @@
-a tag (annotated or lightweight), and then only if it can fast-forward
-. By having the optional leading `+`, you can tell Git to update
-the ref even if it is not allowed by default (e.g., it is not a
--fast-forward.) This does *not* attempt to merge into . See
--EXAMPLES below for details.
+-fast-forward.).
+-+
+-Pushing an empty allows you to delete the ref from
+-the remote repository.
+on the remote side. Whether this is allowed depends on where in
-+`refs/*` the reference lives as described in detail below. Any
-+such update does *not* attempt to merge into . See EXAMPLES
-+below for details.
++`refs/*` the reference lives as described in detail below, in
++those sections "update" means any modifications except deletes, which
++as noted after the next few sections are treated differently.
++
-+The `refs/heads/*` namespace will only accept commit objects, and only
-+if they can be fast-forwarded.
++The `refs/heads/*` namespace will only accept commit objects, and
++updates only if they can be fast-forwarded.
++
+The `refs/tags/*` namespace will accept any kind of object (as
-+commits, trees and blobs can be tagged), and any changes to them will
++commits, trees and blobs can be tagged), and any updates to them will
+be rejected.
++
+It's possible to push any type of object to any namespace outside of
@@ -67,17 +69,26 @@
+new tag object which an existing commit points to.
++
+Tree and blob objects outside of `refs/{tags,heads}/*` will be treated
-+the same way as if they were inside `refs/tags/*`, any modification
[PATCH v5 1/9] fetch: change "branch" to "reference" in --force -h output
The -h output has been referring to the --force command as forcing the
overwriting of local branches, but since "fetch" more generally
fetches all sorts of references in all refs/ namespaces, let's talk
about forcing the update of a a "reference" instead.
This wording was initially introduced in 8320199873 ("Rewrite
builtin-fetch option parsing to use parse_options().", 2007-12-04).
Signed-off-by: Ævar Arnfjörð Bjarmason
---
builtin/fetch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 61bec5d213..b0706b3803 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -114,7 +114,7 @@ static struct option builtin_fetch_options[] = {
N_("append to .git/FETCH_HEAD instead of overwriting")),
OPT_STRING(0, "upload-pack", _pack, N_("path"),
N_("path to upload pack on remote end")),
- OPT__FORCE(, N_("force overwrite of local branch"), 0),
+ OPT__FORCE(, N_("force overwrite of local reference"), 0),
OPT_BOOL('m', "multiple", ,
N_("fetch from multiple remotes")),
OPT_SET_INT('t', "tags", ,
--
2.19.0.rc1.350.ge57e33dbd1
[PATCH v5 6/9] push doc: move mention of "tag " later in the prose
This change will be followed-up with a subsequent change where I'll change both sides of this mention of "tag " to be something that's best read without interruption. To make that change smaller, let's move this mention of "tag " to the end of the "..." section, it's now somewhere in the middle. Signed-off-by: Ævar Arnfjörð Bjarmason --- Documentation/git-push.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt index 83e499ee97..71c78ac1a4 100644 --- a/Documentation/git-push.txt +++ b/Documentation/git-push.txt @@ -80,8 +80,6 @@ a tag (annotated or lightweight), and then only if it can fast-forward the ref even if it is not allowed by default (e.g., it is not a fast-forward.). + -`tag ` means the same as `refs/tags/:refs/tags/`. -+ Pushing an empty allows you to delete the ref from the remote repository. + @@ -89,6 +87,8 @@ The special refspec `:` (or `+:` to allow non-fast-forward updates) directs Git to push "matching" branches: for every branch that exists on the local side, the remote side is updated if a branch of the same name already exists on the remote side. ++ +`tag ` means the same as `refs/tags/:refs/tags/`. --all:: Push all branches (i.e. refs under `refs/heads/`); cannot be -- 2.19.0.rc1.350.ge57e33dbd1
[PATCH v5 7/9] push doc: correct lies about how push refspecs work
There's complex rules governing whether a push is allowed to take
place depending on whether we're pushing to refs/heads/*, refs/tags/*
or refs/not-that/*. See is_branch() in refs.c, and the various
assertions in refs/files-backend.c. (e.g. "trying to write non-commit
object %s to branch '%s'").
This documentation has never been quite correct, but went downhill
after dbfeddb12e ("push: require force for refs under refs/tags/",
2012-11-29) when we started claiming that couldn't be a tag
object, which is incorrect. After some of the logic in that patch was
changed in 256b9d70a4 ("push: fix "refs/tags/ hierarchy cannot be
updated without --force"", 2013-01-16) the docs weren't updated, and
we've had some version of documentation that confused whether
was a tag or not with whether would accept either an annotated
tag object or the commit it points to.
This makes the intro somewhat more verbose & complex, perhaps we
should have a shorter description here and split the full complexity
into a dedicated section. Very few users will find themselves needing
to e.g. push blobs or trees to refs/custom-namespace/* (or blobs or
trees at all), and that could be covered separately as an advanced
topic.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
Documentation/git-push.txt | 52 --
Documentation/gitrevisions.txt | 7 +++--
2 files changed, 48 insertions(+), 11 deletions(-)
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 71c78ac1a4..f345bd30fc 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -74,14 +74,50 @@ without any `` on the command line. Otherwise,
missing
`:` means to update the same ref as the ``.
+
The object referenced by is used to update the reference
-on the remote side. By default this is only allowed if is not
-a tag (annotated or lightweight), and then only if it can fast-forward
-. By having the optional leading `+`, you can tell Git to update
-the ref even if it is not allowed by default (e.g., it is not a
-fast-forward.).
-+
-Pushing an empty allows you to delete the ref from
-the remote repository.
+on the remote side. Whether this is allowed depends on where in
+`refs/*` the reference lives as described in detail below, in
+those sections "update" means any modifications except deletes, which
+as noted after the next few sections are treated differently.
++
+The `refs/heads/*` namespace will only accept commit objects, and
+updates only if they can be fast-forwarded.
++
+The `refs/tags/*` namespace will accept any kind of object (as
+commits, trees and blobs can be tagged), and any updates to them will
+be rejected.
++
+It's possible to push any type of object to any namespace outside of
+`refs/{tags,heads}/*`. In the case of tags and commits, these will be
+treated as if they were the commits inside `refs/heads/*` for the
+purposes of whether the update is allowed.
++
+I.e. a fast-forward of commits and tags outside `refs/{tags,heads}/*`
+is allowed, even in cases where what's being fast-forwarded is not a
+commit, but a tag object which happens to point to a new commit which
+is a fast-forward of the commit the last tag (or commit) it's
+replacing. Replacing a tag with an entirely different tag is also
+allowed, if it points to the same commit, as well as pushing a peeled
+tag, i.e. pushing the commit that existing tag object points to, or a
+new tag object which an existing commit points to.
++
+Tree and blob objects outside of `refs/{tags,heads}/*` will be treated
+the same way as if they were inside `refs/tags/*`, any update of them
+will be rejected.
++
+All of the rules described above about what's not allowed as an update
+can be overridden by adding an the optional leading `+` to a refspec
+(or using `--force` command line option). The only exception to this
+is that no amount of forcing will make the `refs/heads/*` namespace
+accept a non-commit object. Hooks and configuration can also override
+or amend these rules, see e.g. `receive.denyNonFastForwards` in
+linkgit:git-config[1] and`pre-receive` and `update` in
+linkgit:githooks[5].
++
+Pushing an empty allows you to delete the ref from the
+remote repository. Deletions are always accepted without a leading `+`
+in the refspec (or `--force`), except when forbidden by configuration
+or hooks. See `receive.denyDeletes` in linkgit:git-config[1] and
+`pre-receive` and `update` in linkgit:githooks[5].
+
The special refspec `:` (or `+:` to allow non-fast-forward updates)
directs Git to push "matching" branches: for every branch that exists on
diff --git a/Documentation/gitrevisions.txt b/Documentation/gitrevisions.txt
index 1f6cceaefb..d407b7dee1 100644
--- a/Documentation/gitrevisions.txt
+++ b/Documentation/gitrevisions.txt
@@ -19,9 +19,10 @@ walk the revision graph (such as linkgit:git-log[1]), all
commits which are
reachable from that commit. For commands that walk the revision graph one can
also specify a range of
Re: [PATCH 0/3] doc-diff: add "clean" mode & fix portability problem
On Fri, Aug 31, 2018 at 02:33:15AM -0400, Eric Sunshine wrote: > This series replaces Peff's solo patch[1] which updates "make clean" to > remove doc-diff's temporary directory. Rather than imbuing the Makefile > with knowledge specific to doc-diff's internals, this series adds a > "clean" mode to doc-diff which removes its temporary worktree and > generated files, and has "make clean" invoke that instead. It also fixes > a portability problem which prevented doc-diff from working on MacOS and > FreeBSD. Thanks. I left a few comments, but this looks good to me as-is. -Peff
Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on "make clean"
On Fri, Aug 31, 2018 at 02:33:18AM -0400, Eric Sunshine wrote: > doc-diff creates a temporary working tree (git-worktree) and generates a > bunch of temporary files which it does not remove since they act as a > cache to speed up subsequent runs. Although doc-diff's working tree and > generated files are not strictly build products of the Makefile (which, > itself, never runs doc-diff), as a convenience, update "make clean" to > clean up doc-diff's working tree and generated files along with other > development detritus normally removed by "make clean". Makes sense. > diff --git a/Documentation/Makefile b/Documentation/Makefile > index a42dcfc745..26e268ae8d 100644 > --- a/Documentation/Makefile > +++ b/Documentation/Makefile > @@ -332,6 +332,7 @@ clean: > $(RM) SubmittingPatches.txt > $(RM) $(cmds_txt) $(mergetools_txt) *.made > $(RM) manpage-base-url.xsl > + '$(SHELL_PATH_SQ)' ./doc-diff --clean This spelling took me by surprise. The doc-diff script itself specifies /bin/sh, and we do not build it, so the #! line is never replaced. I guess we are leaving it to people on exotic shells to run "$their_sh doc-diff" in the first place. That's probably OK, since it should work out of the box on most /bin/sh instances, and people on other platforms aren't that likely to even run it. I don't think the script does anything complicated that would choke a lesser /bin/sh. But it doesn't hurt to be defensive, since this bit of the Makefile will be run for everyone, whether they care about doc-diff or not. So that all makes sense. I initially wrote this to suggest that we call out this subtlety in the commit message. But I see this is based on existing instances from ee7ec2f9de (documentation: Makefile accounts for SHELL_PATH setting, 2009-03-22). So maybe I am just showing my ignorance. ;) -Peff
Re: [PATCH 2/3] doc-diff: add --clean mode to remove temporary working gunk
On Fri, Aug 31, 2018 at 02:33:17AM -0400, Eric Sunshine wrote: > As part of its operation, doc-diff creates a bunch of temporary > working files and holds onto them in order to speed up subsequent > invocations. These files are never deleted. Moreover, it creates a > temporary working tree (via git-wortkree) which likewise never gets > removed. > > Without knowing the implementation details of the tool, a user may not > know how to clean up manually afterward. Worse, the user may find it > surprising and alarming to discover a working tree which s/he did not > create explicitly. > > To address these issues, add a --clean mode which removes the > temporary working tree and deletes all generated files. That sounds like a good plan. I like keeping the complexity here in the script. > diff --git a/Documentation/doc-diff b/Documentation/doc-diff > index c2906eac5e..f397fd229b 100755 > --- a/Documentation/doc-diff > +++ b/Documentation/doc-diff > @@ -2,20 +2,25 @@ > > OPTIONS_SPEC="\ > doc-diff [options] [-- ] > +doc-diff (-c|--clean) > -- > j=n parallel argument to pass to make > fforce rebuild; do not rely on cached results > +c,clean cleanup temporary working files > " This will cause parseopt to normalize "--clean" to "-c" (along with "--cle", etc). > parallel= > force= > +clean= > while test $# -gt 0 > do > case "$1" in > -j) > parallel=$2; shift ;; > + -c|--clean) > + clean=t ;; So this part can just test for "-c". AFAICT this is how "rev-parse --parseopt" has always worked, though the documentation is quite unclear. Other scripts seem to also use these redundant long options. I'm not opposed to including it as a defensive measure (or simply an annotation for the reader). > +cd_to_toplevel > +tmp=Documentation/tmp-doc-diff > + > +if test -n "$clean" > +then > + test $# -eq 0 || usage > + git worktree remove --force "$tmp/worktree" 2>/dev/null > + rm -rf "$tmp" > + exit 0 > +fi And this matches what I'd expect "--clean" to do. Makes sense. -Peff
Re: What's cooking in git.git (Aug 2018, #06; Wed, 29)
Elijah Newren writes: >> Suggestions for a better rewrite is very much appreciated. > > That makes sense. I'm not sure I can concisely convey all the right > points, but here's a stab at rewording: > > Recent addition of "directory rename" heuristics to the > merge-recursive backend makes the command susceptible to false > positives and false negatives. In the context of "git am -3", which > does not know about surrounding unmodified paths and thus cannot > inform the merge machinery about the full trees involved, this risk is > particularly severe. As such, the heuristic is disabled for "git am > -3" to keep the machinery "more stupid but predictable". Excellent. Let me use that when merging it to 'next'.
Re: [PATCH 1/3] doc-diff: fix non-portable 'man' invocation
On Fri, Aug 31, 2018 at 02:33:16AM -0400, Eric Sunshine wrote: > doc-diff invokes 'man' with the -l option to force "local" mode, > however, neither MacOS nor FreeBSD recognize this option. On those > platforms, if the argument to 'man' contains a slash, it is > automatically interpreted as a file specification, so a "local"-like > mode is not needed. And, it turns out, 'man' which does support -l > falls back to enabling -l automatically if it can't otherwise find a > manual entry corresponding to the argument. Since doc-diff always > passes an absolute path of the nroff source file to 'man', the -l > option kicks in anyhow, despite not being specified explicitly. > Therefore, make the invocation portable to the various platforms by > simply dropping -l. Neat. Today I learned. Confirmed that this works just fine without "-l" on my system (and that "./foo.1" is an easy alternative to "man -l" on other systems). -Peff
Re: [PATCH 2/5] t5303: test some corrupt deltas
On Fri, Aug 31, 2018 at 08:33:26AM -0700, Junio C Hamano wrote: > Johannes Schindelin writes: > > >> Would "echo base >base" give us 5-byte long base even on Windows? > > > > Please note that Unix shell scripting is a foreign thing on Windows. As > > such, there is not really any "native" shell we can use [*1*], and > > Yeah, I know that; otherwise I wouldn't have asked. Because ... > > > therefore we use MSYS2's Bash which outputs Unix line endings. > > ... I didn't know what MSYS folks chose, and/or if you have chosen > to tweak their choice, and/or if you switched to somebody else's shell > (e.g. busybox) and/or you chose to tweak what they do out of the box, > it was worth asking and getting yes/no question. You do not have to > tell me why I should be asking. > > So instead of typing 3 lines, you can just say "yes we use echo that > emulates Unix". I actually found Dscho's response much more informative than a simple yes/no. At any rate, it sounds like we are probably OK with echo, but I think it is still worth doing the defensive patch-on-top that I posted. -Peff
Re: A rebase regression in Git 2.18.0
Hi Dscho, On Fri, Aug 31, 2018 at 3:12 AM Johannes Schindelin wrote: > On Thu, 30 Aug 2018, Elijah Newren wrote: > > On Tue, Aug 28, 2018 at 9:58 AM Junio C Hamano wrote: > > > Elijah Newren writes: ... > > > I'd say this is the only practical solution, before you deprecate > > > the "pipe format-patch output to am -3" style of "git rebase" (and > > > optionally replace with something else). > > > > I posted a patch a while back to add an --am flag to "git rebase", > > make "--am" be implied by options which are still am-specific > > (--whitespace, --committer-date-is-author-date, and -C), and change > > --merge to be the default. > > Didn't you also post a patch to fold --merge into the --interactive > backend? What's your current state of thinking about this? Yes. I updated it once or twice, but it had conflicts with the GSoC projects each time, so I decided to just hold off on it a bit longer. I'm still planning to resubmit this once the GSoC projects merge down. > As to switching from --am as the default: I still think that --am has > serious speed advantages over --merge (or for that matter, --interactive). > I have no numbers to back that up, though, and I am currently really busy > with working on the CI, so I won't be able to measure these numbers, > either... Yep, we talked about this before and you mentioned that the rewrite in C should bring some performance improvements, and we agreed that merge-recursive is probably the next issue performance-wise. I think it's at least worth measuring what the approximate performance differences are with the rewrite of rebase in C, and posting an RFC with that info. If the answer comes back that we need to do more optimization before we switch the default, that's fine. > Also please note: I converted the `am` backend to pure C (it is waiting at > https://github.com/gitgitgadget/git/pull/24, to be submitted after the > v2.19.0 RC period). Switching to `--merge` as the default would force me > to convert that backend, too ;-) Not if git-rebase--merge is deleted and --merge is implemented on top of the interactive backend as an implicitly_interactive case. In fact, that's probably the simplest way to "convert" that backend to C. Anyway, since I plan to submit that change first, we should be good. > > I'll post it as an RFC again after the various rebase-rewrite series > > have settled and merged down...along with my other rebase cleanups > > that I was waiting on to avoid conflicts with GSoC stuff. > > Thanks for waiting! Please note that I am interested, yet I will be on > vacation for a couple of weeks in September. Don't let that stop you, > though! Enjoy your vacation! > > > The whole point of "am -3" is to do _better_ than just "patch" with > > > minimum amount of information available on the pre- and post- image > > > blobs, without knowing the remainder of the tree that the patch did > > > not touch. It is not surprising that the heuristics that look at > > > the unchanging part of the tree to infer renames that may or may not > > > exist guesses incorrectly, either with false positive or negative. > > > In the context of "rebase", we always have all the trees that are > > > involved. We should be able to do better than "am -3". > > Right. I think that Elijah's right, and --merge is that "do better" > solution. Cool, good to see others seem to agree on the direction I'd like to see things move.
Re: [PATCH 0/3] doc-diff: add "clean" mode & fix portability problem
Eric Sunshine writes: > This series replaces Peff's solo patch[1] which updates "make clean" to > remove doc-diff's temporary directory. Rather than imbuing the Makefile > with knowledge specific to doc-diff's internals, this series adds a > "clean" mode to doc-diff which removes its temporary worktree and > generated files, and has "make clean" invoke that instead. That sounds like a better approach. > It also fixes > a portability problem which prevented doc-diff from working on MacOS and > FreeBSD. > > [1]: > https://public-inbox.org/git/20180830195546.ga22...@sigill.intra.peff.net/ > > Eric Sunshine (3): > doc-diff: fix non-portable 'man' invocation > doc-diff: add --clean mode to remove temporary working gunk > doc/Makefile: drop doc-diff worktree and temporary files on "make > clean" > > Documentation/Makefile | 1 + > Documentation/doc-diff | 21 + > 2 files changed, 18 insertions(+), 4 deletions(-)
Re: [PATCH v3 05/11] t0027: make hash size independent
On Fri, Aug 31, 2018 at 2:21 PM Torsten Bögershausen wrote:
> > diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
> > @@ -14,11 +14,13 @@ compare_files () {
> > compare_ws_file () {
> > + tmp=$2.tmp
> > act=$pfx.actual.$3
> > - tr '\015\000abcdef0123456789' QN0 <"$2" >"$exp" &&
> > + tr '\015\000abcdef0123456789' QN0 <"$2" >"$tmp" &&
> > tr '\015\000abcdef0123456789' QN0 <"$3" >"$act" &&
> > + sed -e "s/*/$ZERO_OID/" "$tmp" >"$exp" &&
>
> Out of interest: why do we use a "tmp" file here?
> Would it make more sense to chain the 'tr' with 'sed' and skip the
> tmp file ?
>
> tr '\015\000abcdef0123456789' QN0 <"$2" |
> sed -e "s/*/$ZERO_OID/" >"$exp" &&
>
> Yes, we will loose the exit status of 'tr', I think.
> How important is the exit status ?
As far as I understand, it is only Git commands for which we worry
about losing the exit status upstream in pipes. System utilities, on
the other hand, are presumed to be bug-free, thus we don't mind having
them upstream.
A different question is why does this need to run both 'tr' and 'sed'
when 'sed itself could do the entire job since 'sed' has 'tr'
functionality built in (see sed's "y" command)?
Re: [PATCH] config.mak.uname: resolve FreeBSD iconv-related compilation warning
On Fri, Aug 31, 2018 at 7:54 AM Ævar Arnfjörð Bjarmason wrote: > On Fri, Aug 31, 2018 at 11:52 AM Eric Sunshine > wrote: > > OLD_ICONV has long been needed by FreeBSD so config.mak.uname defines > > it unconditionally. However, recent versions do not need it, and its > > presence results in compilation warnings. Resolve this issue by defining > > OLD_ICONV only for older FreeBSD versions. > > This seems sane, but just for context is FreeBSD ports itself just > compiling without iconv entirely? > > I have little clue about how ports works, but just noticed that > they're not monkeypatching in OLD_ICONV there. My experience with FreeBSD ports is pretty limited too, but, as I discovered in [1], they run Git's configure script, so OLD_ICONV is determined dynamically, as far as I can tell. [1]: https://public-inbox.org/git/CAPig+cTEGtsmUyoYsKEx+erMsXKm5=c6tjunageky2pcgw1...@mail.gmail.com/
Re: [PATCH v3 05/11] t0027: make hash size independent
On Wed, Aug 29, 2018 at 12:56:36AM +, brian m. carlson wrote:
> We transform various object IDs into all-zero object IDs for comparison.
> Adjust the length as well so that this works for all hash algorithms.
>
> Signed-off-by: brian m. carlson
> ---
> t/t0027-auto-crlf.sh | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
> index beb5927f77..0f1235d9d1 100755
> --- a/t/t0027-auto-crlf.sh
> +++ b/t/t0027-auto-crlf.sh
> @@ -14,11 +14,13 @@ compare_files () {
> compare_ws_file () {
> pfx=$1
> exp=$2.expect
> + tmp=$2.tmp
> act=$pfx.actual.$3
> - tr '\015\000abcdef0123456789' QN0 <"$2" >"$exp" &&
> + tr '\015\000abcdef0123456789' QN0 <"$2" >"$tmp" &&
> tr '\015\000abcdef0123456789' QN0 <"$3" >"$act" &&
> + sed -e "s/*/$ZERO_OID/" "$tmp" >"$exp" &&
> test_cmp "$exp" "$act" &&
> - rm "$exp" "$act"
> + rm "$exp" "$act" "$tmp"
> }
>
> create_gitattributes () {
I only managed to review the changes in t0027.
Out of interest: why do we use a "tmp" file here?
Would it make more sense to chain the 'tr' with 'sed' and skip the
tmp file ?
tr '\015\000abcdef0123456789' QN0 <"$2" |
sed -e "s/*/$ZERO_OID/" >"$exp" &&
Yes, we will loose the exit status of 'tr', I think.
How important is the exit status ?
I don't know, hopefully someone with more experience/knowledge
about shell scripting can help me out here.
Re: [GSoC][PATCH v8 00/20] Convert "git stash" to C builtin
Paul-Sebastian Ungureanu writes:
> This a new iteration of `stash.c`. What is new?
>
> * Some commits got squashed. The commit related to replacing
> `git apply` child process was dropped since it wasn't the best
> idea.
>
> * In v7, there was a bug [1] related to config `git stash show`
> The bug was fixed and a test file was added for this.
>
> * Fixed `git stash [push]` [2]. In v7, `git stash -q drop` would
> act like `git stash push -q drop`.
>
> * Fixed coding-style nits. Verified that messages are marked
> for translation and are going to the correct output stream.
>
> * Fixed one memory leak (related to `strbuf_detach`).
>
> * Simplified the code a little bit.
Also worth noting.
* Rebased on a recent 'master', in which the calling convention
for functions like dir_path_match() has been updated. It won't
compile when applied to anything older than dc0f6f9e ("Merge
branch 'nd/no-the-index'", 2018-08-20).
Re: [PATCH 3/3] wt-status.c: Set the commitable flag in the collect phase.
On Friday, August 31, 2018 9:54:50 AM MST Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason writes: > >> Leave the setting of the commitable flag in show_merge_in_progress. If > >> a check for merged commits is moved to the collect phase then other > >> --dry-run tests fail. > > "Some tests fail" is not a good explanation why you keep the setting > of commitable bit in the "show" codepath. The test coverage may not > be thorough, and the tests that fail might be expecting wrong things. > I didn't figure it was, but I haven't yet figured out how to explain what what I saw last evening. I wanted to send something out to get feedback from someone who may know the code far better than I. > The change in this patch makes the internal "diff-index" invocation > responsible for setting the commitable bit. This is better for non > merge commits than the current code because the current code only > sets the commitable bit when longstatus is asked for (and the code > to show the longstatus detects changes to be committed), so the > short-form will not have chance to set the bit, but the internal > "diff-index" is what determines if the resulting commit would have > difference relative to the HEAD, so it is a better place to make > that decision. > > Merge commits need to be allowed even when the resulting commit > records a tree that is identical to that of the current HEAD > (i.e. we merged a side branch, but we already had all the necessary > changes done on it). So it is insufficient to let "diff-index" > invocation to set the committable bit. Is that why "other --dry-run > tests fail"? What I am getting at is to have a reasonable "because > ..." to explain why "other --dry-run tests fail" after it, to make > it clear to the readers that the failure is not because tests are > checking wrong things but because a specific condition thatwt_status_collect(), isYes > expeted from the code gets violated if we change the code in > show_merge_in_progress() function. Agreed. I'm just green at this code base, and so don't quite know what I should see as opposed to what I do see. > > That brings us to another point. Is there a case where we want to > see s->commitable bit set correctly but we do not make any call to > show_merge_in_progress() function? It is conceivable to have a new > "git commit --dry-run --quiet [[--] ]" mode that is > totally silent but reports if what we have is committable with the > exit status, and for that we would not want to call any show_* > functions. That leads me to suspect that ideally we would want to > see wt_status_collect_changes_index() to be the one that is setting > the commitable bit. Or even its caller wt_status_collect(), which > would give us a better chance of being correct even during the > initial commit. For the "during merge" case, we would need to say > something like > > if (state->merge_in_progress && !has_unmerged(s)) > s->commitable = 1; > I placed the following in wt_status_collect() last evening, and received errors from three early tests in 7501-commit.sh. Thanks for a hint. if (!has_unmerged(s)) s->commitable = 1; Maybe the missing first condition was what I needed. Which leads me to asking: Do you want a preparatory patch moving has_unmerged() further up in the file before adding a reference to has_unmerged() in wt_status_collect(). > but the "state" thing is passed around only among the "print/show" > level of functions in the current code. We might need to merge that > into the wt_status structure to pass it down to the "collect" phase > at the lower level before/while doing so. I dunno. Would you explain what you are thinking for passing moving the "stat" think into wt_status.I haven't figured out how the "collect" sequence, relates to the "print/show" squence. > > Thanks for working on this. I decided sometime back to work on something I didn't know using a process I don't normally use to broaden my experience. I'm enjoying this and hope you don't mind lots of questions from someone new. sps
Re: [PATCH 2/3] Add test for commit --dry-run --short.
On Friday, August 31, 2018 9:26:02 AM MST Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason writes: > > > > Ditto my comment on 1/3 on this. I.e. this changes the failing tests in > > this series from 2 to 3. > > Correct. Thanks for helping Stephen on this topic. I wasn't sure how this situation was normally handled. I will update when I re-roll changes for wt-status.c.
Re: [PATCH 1/8] trace2: create new combined trace facility
On 8/31/2018 12:49 PM, Jeff Hostetler via GitGitGadget wrote:
+ if (tr2key_trace_want(_event)) {
+ struct tr2tls_thread_ctx *ctx = tr2tls_get_self();
+ if (ctx->nr_open_regions <= tr2env_event_depth_wanted) {
This should also compare to TR2_MAX_REGION_NESTING to avoid memory
bounds issues. It may even be worth sending a message that the depth
went beyond the limits.
Thanks,
-Stolee
Re: [PATCH 0/8] WIP: trace2: a new trace facility
On 8/31/2018 12:49 PM, Jeff Hostetler via GitGitGadget wrote: This patch series contains a new trace2 facility that hopefully addresses the recent trace- and structured-logging-related discussions. The intent is to eventually replace the existing trace_ routines (or to route them to the new trace2_ routines) as time permits. I haven't been part of the recent discussions on these logging efforts, but I wanted to mention that I'm excited about the potential here. I want to use this new tracing model to trace how many commits are walked by graph algorithms like paint_down_to_common(). I'm playing with some efforts here, and found one issue when the API is used incorrectly (I'll respond to the patch with the issue). Thanks, -Stolee
Re: [PATCH] config.mak.uname: resolve FreeBSD iconv-related compilation warning
Eric Sunshine wrote: > From: Jonathan Nieder > > OLD_ICONV has long been needed by FreeBSD so config.mak.uname defines > it unconditionally. However, recent versions do not need it, and its > presence results in compilation warnings. Resolve this issue by defining > OLD_ICONV only for older FreeBSD versions. > > Specifically, revision r281550[1], which is part of FreeBSD 11, removed > the need for OLD_ICONV, and r282275[2] back-ported that change to 10.2. > Versions prior to 10.2 do need it. > > [1] > https://github.com/freebsd/freebsd/commit/b0813ee288f64f677a2cebf7815754b027a8215b > [2] > https://github.com/freebsd/freebsd/commit/b709ec868adb5170d09bc5a66b18d0e0d5987ab6 > > [es: commit message; tweak version check to distinguish 10.x versions] > > Signed-off-by: Jonathan Nieder > Signed-off-by: Eric Sunshine > --- > config.mak.uname | 12 +++- > 1 file changed, 11 insertions(+), 1 deletion(-) I think it makes sense for you to take credit for this one. You noticed the original problem, tested on FreeBSD, wrote the explanation, and figured out the firstword hackery. All I did was to say "somebody should fix this" and run "git log -S" a few times. In any event, Reviewed-by: Jonathan Nieder
Re: [PATCH 3/3] wt-status.c: Set the commitable flag in the collect phase.
Ævar Arnfjörð Bjarmason writes: > On Fri, Aug 31 2018, Stephen P. Smith wrote: > >> In an update to fix a bug with "commit --dry-run" it was found that >> the commitable flag was broken. The update was, at the time, >> accepted as it was better than the previous version. > > What update is this? I.e. git.git commit id? See the "or this invocation > of `git show`" part of SubmittingPatches for how to quote it in the > commit message. > >> Since the set of the flag had been done in wt_longstatus_print_updated, >> set the flag in wt_status_collect_updated_cb. >> >> Set the commitable flag in wt_status_collect_changes_initial to keep >> from introducing a rebase regression. >> >> Leave the setting of the commitable flag in show_merge_in_progress. If >> a check for merged commits is moved to the collect phase then other >> --dry-run tests fail. "Some tests fail" is not a good explanation why you keep the setting of commitable bit in the "show" codepath. The test coverage may not be thorough, and the tests that fail might be expecting wrong things. The change in this patch makes the internal "diff-index" invocation responsible for setting the commitable bit. This is better for non merge commits than the current code because the current code only sets the commitable bit when longstatus is asked for (and the code to show the longstatus detects changes to be committed), so the short-form will not have chance to set the bit, but the internal "diff-index" is what determines if the resulting commit would have difference relative to the HEAD, so it is a better place to make that decision. Merge commits need to be allowed even when the resulting commit records a tree that is identical to that of the current HEAD (i.e. we merged a side branch, but we already had all the necessary changes done on it). So it is insufficient to let "diff-index" invocation to set the committable bit. Is that why "other --dry-run tests fail"? What I am getting at is to have a reasonable "because ..." to explain why "other --dry-run tests fail" after it, to make it clear to the readers that the failure is not because tests are checking wrong things but because a specific condition that is expeted from the code gets violated if we change the code in show_merge_in_progress() function. That brings us to another point. Is there a case where we want to see s->commitable bit set correctly but we do not make any call to show_merge_in_progress() function? It is conceivable to have a new "git commit --dry-run --quiet [[--] ]" mode that is totally silent but reports if what we have is committable with the exit status, and for that we would not want to call any show_* functions. That leads me to suspect that ideally we would want to see wt_status_collect_changes_index() to be the one that is setting the commitable bit. Or even its caller wt_status_collect(), which would give us a better chance of being correct even during the initial commit. For the "during merge" case, we would need to say something like if (state->merge_in_progress && !has_unmerged(s)) s->commitable = 1; but the "state" thing is passed around only among the "print/show" level of functions in the current code. We might need to merge that into the wt_status structure to pass it down to the "collect" phase at the lower level before/while doing so. I dunno. Thanks for working on this.
[PATCH 4/8] trace2: demonstrate trace2 child process classification
From: Jeff Hostetler
Classify editory, pager, and sub-process child processes.
The former two can be used to identify interactive commands,
for example.
Signed-off-by: Jeff Hostetler
---
editor.c | 1 +
pager.c | 1 +
sub-process.c | 1 +
3 files changed, 3 insertions(+)
diff --git a/editor.c b/editor.c
index 9a9b4e12d1..29707de198 100644
--- a/editor.c
+++ b/editor.c
@@ -66,6 +66,7 @@ int launch_editor(const char *path, struct strbuf *buffer,
const char *const *en
p.argv = args;
p.env = env;
p.use_shell = 1;
+ p.trace2_child_class = "editor";
if (start_command() < 0)
return error("unable to start editor '%s'", editor);
diff --git a/pager.c b/pager.c
index a768797fcf..4168460ae9 100644
--- a/pager.c
+++ b/pager.c
@@ -100,6 +100,7 @@ void prepare_pager_args(struct child_process
*pager_process, const char *pager)
argv_array_push(_process->args, pager);
pager_process->use_shell = 1;
setup_pager_env(_process->env_array);
+ pager_process->trace2_child_class = "pager";
}
void setup_pager(void)
diff --git a/sub-process.c b/sub-process.c
index 8d2a1707cf..3f4af93555 100644
--- a/sub-process.c
+++ b/sub-process.c
@@ -88,6 +88,7 @@ int subprocess_start(struct hashmap *hashmap, struct
subprocess_entry *entry, co
process->out = -1;
process->clean_on_exit = 1;
process->clean_on_exit_handler = subprocess_exit_handler;
+ process->trace2_child_class = "subprocess";
err = start_command(process);
if (err) {
--
gitgitgadget
[PATCH 1/8] trace2: create new combined trace facility
From: Jeff Hostetler
Create trace2 API allowing event-based tracing. This will hopefully
eventually replace the existing trace API.
Create GIT_TR2 trace-key to replace GIT_TRACE, GIT_TR2_PERFORMANCE to
replace GIT_TRACE_PERFORMANCE, and a new trace-key GIT_TR2_EVENT to
generate JSON data for telemetry purposes. Other structured formats
can easily be added later using this new existing model.
Define a higher-level event API that selectively writes to all of the
new GIT_TR2_* targets (depending on event type) without needing to call
different trace_printf*() or trace_performance_*() routines.
The API defines both fixed-field and printf-style functions.
The trace2 performance tracing includes thread-specific function
nesting and timings.
Signed-off-by: Jeff Hostetler
---
Makefile |1 +
cache.h |1 +
trace2.c | 1592 ++
trace2.h | 214
4 files changed, 1808 insertions(+)
create mode 100644 trace2.c
create mode 100644 trace2.h
diff --git a/Makefile b/Makefile
index 5a969f5830..88ae7afdd4 100644
--- a/Makefile
+++ b/Makefile
@@ -974,6 +974,7 @@ LIB_OBJS += tag.o
LIB_OBJS += tempfile.o
LIB_OBJS += tmp-objdir.o
LIB_OBJS += trace.o
+LIB_OBJS += trace2.o
LIB_OBJS += trailer.o
LIB_OBJS += transport.o
LIB_OBJS += transport-helper.o
diff --git a/cache.h b/cache.h
index 4d014541ab..38f0cd2ba0 100644
--- a/cache.h
+++ b/cache.h
@@ -9,6 +9,7 @@
#include "gettext.h"
#include "convert.h"
#include "trace.h"
+#include "trace2.h"
#include "string-list.h"
#include "pack-revindex.h"
#include "hash.h"
diff --git a/trace2.c b/trace2.c
new file mode 100644
index 00..13d5c85366
--- /dev/null
+++ b/trace2.c
@@ -0,0 +1,1592 @@
+#include "cache.h"
+#include "config.h"
+#include "json-writer.h"
+#include "quote.h"
+#include "run-command.h"
+#include "sigchain.h"
+#include "thread-utils.h"
+#include "version.h"
+
+/*
+ * TODO remove this section header
+ */
+
+static struct strbuf tr2sid_buf = STRBUF_INIT;
+static int tr2sid_nr_git_parents;
+
+/*
+ * Compute a "unique" session id (SID) for the current process. All events
+ * from this process will have this label. If we were started by another
+ * git instance, use our parent's SID as a prefix and count the number of
+ * nested git processes. (This lets us track parent/child relationships
+ * even if there is an intermediate shell process.)
+ */
+static void tr2sid_compute(void)
+{
+ uint64_t us_now;
+ const char *parent_sid;
+
+ if (tr2sid_buf.len)
+ return;
+
+ parent_sid = getenv("SLOG_PARENT_SID");
+ if (parent_sid && *parent_sid) {
+ const char *p;
+ for (p = parent_sid; *p; p++)
+ if (*p == '/')
+ tr2sid_nr_git_parents++;
+
+ strbuf_addstr(_buf, parent_sid);
+ strbuf_addch(_buf, '/');
+ tr2sid_nr_git_parents++;
+ }
+
+ us_now = getnanotime() / 1000;
+ strbuf_addf(_buf, "%"PRIuMAX"-%"PRIdMAX,
+ (uintmax_t)us_now, (intmax_t)getpid());
+
+ setenv("SLOG_PARENT_SID", tr2sid_buf.buf, 1);
+}
+
+/*
+ * TODO remove this section header
+ */
+
+/*
+ * Each thread (including the main thread) has a stack of nested regions.
+ * This is used to indent messages and provide nested perf times. The
+ * limit here is for simplicity. Note that the region stack is per-thread
+ * and not per-trace_key.
+ */
+#define TR2_MAX_REGION_NESTING (100)
+#define TR2_INDENT (2)
+#define TR2_INDENT_LENGTH(ctx) (((ctx)->nr_open_regions - 1) * TR2_INDENT)
+
+#define TR2_MAX_THREAD_NAME (24)
+
+struct tr2tls_thread_ctx {
+ struct strbuf thread_name;
+ uint64_t us_start[TR2_MAX_REGION_NESTING];
+ int nr_open_regions;
+ int thread_id;
+};
+
+static struct tr2tls_thread_ctx *tr2tls_thread_main;
+
+static pthread_mutex_t tr2tls_mutex;
+static pthread_key_t tr2tls_key;
+static int tr2tls_initialized;
+
+static struct strbuf tr2_dots = STRBUF_INIT;
+
+static int tr2_next_child_id;
+static int tr2_next_thread_id;
+
+/*
+ * Create TLS data for the current thread. This gives us a place to
+ * put per-thread data, such as thread start time, function nesting
+ * and a per-thread label for our messages.
+ *
+ * We assume the first thread is "main". Other threads are given
+ * non-zero thread-ids to help distinguish messages from concurrent
+ * threads.
+ *
+ * Truncate the thread name if necessary to help with column alignment
+ * in printf-style messages.
+ */
+static struct tr2tls_thread_ctx *tr2tls_create_self(const char *thread_name)
+{
+ struct tr2tls_thread_ctx *ctx = xcalloc(1, sizeof(*ctx));
+
+ /*
+* Implicitly
[PATCH 5/8] trace2: demonstrate instrumenting do_read_index
From: Jeff Hostetler
Signed-off-by: Jeff Hostetler
---
read-cache.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/read-cache.c b/read-cache.c
index 7b1354d759..7a31ac4da8 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1867,6 +1867,8 @@ static void tweak_split_index(struct index_state *istate)
static void post_read_index_from(struct index_state *istate)
{
+ trace2_data_intmax("index", "cache_nr", istate->cache_nr);
+
check_ce_order(istate);
tweak_untracked_cache(istate);
tweak_split_index(istate);
@@ -2012,7 +2014,9 @@ int read_index_from(struct index_state *istate, const
char *path,
if (istate->initialized)
return istate->cache_nr;
+ trace2_region_enter("do_read_index");
ret = do_read_index(istate, path, 0);
+ trace2_region_leave("do_read_index");
trace_performance_since(start, "read cache %s", path);
split_index = istate->split_index;
@@ -2028,7 +2032,9 @@ int read_index_from(struct index_state *istate, const
char *path,
base_oid_hex = oid_to_hex(_index->base_oid);
base_path = xstrfmt("%s/sharedindex.%s", gitdir, base_oid_hex);
+ trace2_region_enter("do_read_index");
ret = do_read_index(split_index->base, base_path, 1);
+ trace2_region_leave("do_read_index");
if (oidcmp(_index->base_oid, _index->base->oid))
die("broken index, expect %s in %s, got %s",
base_oid_hex, base_path,
--
gitgitgadget
[PATCH 0/8] WIP: trace2: a new trace facility
This patch series contains a new trace2 facility that hopefully addresses the recent trace- and structured-logging-related discussions. The intent is to eventually replace the existing trace_ routines (or to route them to the new trace2_ routines) as time permits. This draft adds new trace2_ calls and leaves most of the original trace_ calls in place. Subsequent drafts will address this. This version should be considered a replacement for my earlier structured logging patch series [1]. It addresses Jonathan Nieder's, Ben Peart's, Peff's, and Junio's comments [2,3,4,5] about merging the existing tracing routines, ease of use, progressive logging rather than logging at the end of the program, hiding all JSON details inside the trace2_ routines, and leaving an opening for additional formats (protobuf or whatever). It also adds a nested performance tracing feature similar to Duy's suggestion in [6]. This version adds per-thread nesting and marks each event with a thread name. [1] https://public-inbox.org/git/20180713165621.52017-1-...@jeffhostetler.com/ [2] https://public-inbox.org/git/20180821044724.ga219...@aiede.svl.corp.google.com/ [3] https://public-inbox.org/git/13302a8c-a114-c3a7-65df-55f47f902...@gmail.com/ [4] https://public-inbox.org/git/20180814195456.ge28...@sigill.intra.peff.net/ [5] https://public-inbox.org/git/xmqqeff0zn53@gitster-ct.c.googlers.com/ [6] https://public-inbox.org/git/20180818144128.19361-2-pclo...@gmail.com/ Cc: gitster@pobox.comCc: peff@peff.netCc: peartben@gmail.comCc: jrnieder@gmail.comCc: pclo...@gmail.com Jeff Hostetler (8): trace2: create new combined trace facility trace2: add trace2 to main trace2: demonstrate trace2 regions in wt-status trace2: demonstrate trace2 child process classification trace2: demonstrate instrumenting do_read_index trace2: demonstrate instrumenting threaded preload_index trace2: demonstrate setting sub-command parameter in checkout trace2: demonstrate use of regions in read_directory_recursive Makefile |1 + builtin/checkout.c |5 + cache.h|1 + compat/mingw.h |3 +- dir.c |3 + editor.c |1 + git-compat-util.h |7 + git.c |9 +- pager.c|1 + preload-index.c| 10 + read-cache.c |6 + repository.c |2 + run-command.c |8 +- run-command.h |5 + sub-process.c |1 + trace2.c | 1592 trace2.h | 214 ++ usage.c|5 + wt-status.c| 14 +- 19 files changed, 1882 insertions(+), 6 deletions(-) create mode 100644 trace2.c create mode 100644 trace2.h base-commit: 2f743933341f27603550fbf383a34dfcfd38 Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-29%2Fjeffhostetler%2Fml-trace2-v0-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-29/jeffhostetler/ml-trace2-v0-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/29 -- gitgitgadget
[PATCH 6/8] trace2: demonstrate instrumenting threaded preload_index
From: Jeff Hostetler
Add trace2_region_enter() and _leave() around the entire preload_index()
call. Also add thread-specific events in the preload_thread() threadproc.
Signed-off-by: Jeff Hostetler
---
preload-index.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/preload-index.c b/preload-index.c
index 71cd2437a3..2483d92c61 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -40,10 +40,14 @@ static void *preload_thread(void *_data)
struct cache_entry **cep = index->cache + p->offset;
struct cache_def cache = CACHE_DEF_INIT;
+ trace2_thread_start("preload_thread");
+
nr = p->nr;
if (nr + p->offset > index->cache_nr)
nr = index->cache_nr - p->offset;
+ trace2_printf("preload {offset %7d}{count %7d}", p->offset, nr);
+
do {
struct cache_entry *ce = *cep++;
struct stat st;
@@ -70,6 +74,9 @@ static void *preload_thread(void *_data)
mark_fsmonitor_valid(ce);
} while (--nr > 0);
cache_def_clear();
+
+ trace2_thread_exit();
+
return NULL;
}
@@ -118,6 +125,9 @@ int read_index_preload(struct index_state *index,
{
int retval = read_index(index);
+ trace2_region_enter("preload_index");
preload_index(index, pathspec);
+ trace2_region_leave("preload_index");
+
return retval;
}
--
gitgitgadget
[PATCH 3/8] trace2: demonstrate trace2 regions in wt-status
From: Jeff Hostetler
Add trace2_region_enter() and trace2_region_leave() calls around the
various phases of a status scan. This gives elapsed time for each
phase in the GIT_TR2_PERFORMANCE trace target.
Signed-off-by: Jeff Hostetler
---
wt-status.c | 14 --
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/wt-status.c b/wt-status.c
index 5ffab61015..9e37a73c73 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -726,13 +726,23 @@ static void wt_status_collect_untracked(struct wt_status
*s)
void wt_status_collect(struct wt_status *s)
{
+ trace2_region_enter("status_worktrees");
wt_status_collect_changes_worktree(s);
+ trace2_region_leave("status_worktrees");
- if (s->is_initial)
+ if (s->is_initial) {
+ trace2_region_enter("status_initial");
wt_status_collect_changes_initial(s);
- else
+ trace2_region_leave("status_initial");
+ } else {
+ trace2_region_enter("status_index");
wt_status_collect_changes_index(s);
+ trace2_region_leave("status_index");
+ }
+
+ trace2_region_enter("status_untracked");
wt_status_collect_untracked(s);
+ trace2_region_leave("status_untracked");
}
static void wt_longstatus_print_unmerged(struct wt_status *s)
--
gitgitgadget
[PATCH 7/8] trace2: demonstrate setting sub-command parameter in checkout
From: Jeff Hostetler
Add trace2_param() events in checkout to report whether the command
is switching branches or just checking out a file.
Signed-off-by: Jeff Hostetler
---
builtin/checkout.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 29ef50013d..0934587781 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -251,6 +251,8 @@ static int checkout_paths(const struct checkout_opts *opts,
int errs = 0;
struct lock_file lock_file = LOCK_INIT;
+ trace2_param("subcommand", (opts->patch_mode ? "patch" : "path"));
+
if (opts->track != BRANCH_TRACK_UNSPECIFIED)
die(_("'%s' cannot be used with updating paths"), "--track");
@@ -828,6 +830,9 @@ static int switch_branches(const struct checkout_opts *opts,
void *path_to_free;
struct object_id rev;
int flag, writeout_error = 0;
+
+ trace2_param("subcommand", "switch");
+
memset(_branch_info, 0, sizeof(old_branch_info));
old_branch_info.path = path_to_free = resolve_refdup("HEAD", 0, ,
);
if (old_branch_info.path)
--
gitgitgadget
[PATCH 8/8] trace2: demonstrate use of regions in read_directory_recursive
From: Jeff Hostetler
Add trace2_region_enter() and _leave() calls inside read_directory_recursive()
to show nesting and per-level timing.
TODO Drop this commit or ifdef the calls. It generates too much data to
be in the production version. It is included in this patch series for
illustration purposes only. It is typical of what a developer might do
during a perf investigation.
Signed-off-by: Jeff Hostetler
---
dir.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/dir.c b/dir.c
index aceb0d4869..c7c0654da5 100644
--- a/dir.c
+++ b/dir.c
@@ -1951,6 +1951,8 @@ static enum path_treatment
read_directory_recursive(struct dir_struct *dir,
strbuf_add(, base, baselen);
+ trace2_region_enter("read_directory_recursive:%.*s", baselen, base);
+
if (open_cached_dir(, dir, untracked, istate, , check_only))
goto out;
@@ -2042,6 +2044,7 @@ static enum path_treatment
read_directory_recursive(struct dir_struct *dir,
close_cached_dir();
out:
strbuf_release();
+ trace2_region_leave("read_directory_recursive:%.*s", baselen, base);
return dir_state;
}
--
gitgitgadget
[PATCH 2/8] trace2: add trace2 to main
From: Jeff Hostetler
Signed-off-by: Jeff Hostetler
---
compat/mingw.h| 3 +--
git-compat-util.h | 7 +++
git.c | 9 -
repository.c | 2 ++
run-command.c | 8 +++-
run-command.h | 5 +
usage.c | 5 +
7 files changed, 35 insertions(+), 4 deletions(-)
diff --git a/compat/mingw.h b/compat/mingw.h
index 571019d0bd..606402faeb 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -144,8 +144,7 @@ static inline int fcntl(int fd, int cmd, ...)
errno = EINVAL;
return -1;
}
-/* bash cannot reliably detect negative return codes as failure */
-#define exit(code) exit((code) & 0xff)
+
#define sigemptyset(x) (void)0
static inline int sigaddset(sigset_t *set, int signum)
{ return 0; }
diff --git a/git-compat-util.h b/git-compat-util.h
index 5f2e90932f..c0901d9ec6 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1219,6 +1219,13 @@ static inline int is_missing_file_error(int errno_)
extern int cmd_main(int, const char **);
+/*
+ * Intercept all calls to exit() and route them to trace2 to
+ * optionally emit a message before calling the real exit().
+ */
+int trace2_exit_fl(const char *file, int line, int code);
+#define exit(code) exit(trace2_exit_fl(__FILE__, __LINE__, (code)))
+
/*
* You can mark a stack variable with UNLEAK(var) to avoid it being
* reported as a leak by tools like LSAN or valgrind. The argument
diff --git a/git.c b/git.c
index c27c38738b..cc56279a8c 100644
--- a/git.c
+++ b/git.c
@@ -331,6 +331,8 @@ static int handle_alias(int *argcp, const char ***argv)
argv_array_push(, alias_string + 1);
argv_array_pushv(, (*argv) + 1);
+ trace2_alias(alias_command, child.args.argv);
+
ret = run_command();
if (ret >= 0) /* normal exit */
exit(ret);
@@ -365,6 +367,8 @@ static int handle_alias(int *argcp, const char ***argv)
/* insert after command name */
memcpy(new_argv + count, *argv + 1, sizeof(char *) * *argcp);
+ trace2_alias(alias_command, new_argv);
+
*argv = new_argv;
*argcp += count - 1;
@@ -413,6 +417,7 @@ static int run_builtin(struct cmd_struct *p, int argc,
const char **argv)
setup_work_tree();
trace_argv_printf(argv, "trace: built-in: git");
+ trace2_command(p->cmd);
validate_cache_entries(_index);
status = p->fn(argc, argv, prefix);
@@ -719,6 +724,8 @@ int cmd_main(int argc, const char **argv)
cmd = slash + 1;
}
+ trace2_start(argv);
+
trace_command_performance(argv);
/*
@@ -782,5 +789,5 @@ int cmd_main(int argc, const char **argv)
fprintf(stderr, _("failed to run command '%s': %s\n"),
cmd, strerror(errno));
- return 1;
+ return trace2_exit(1);
}
diff --git a/repository.c b/repository.c
index 5dd1486718..c169f61ccd 100644
--- a/repository.c
+++ b/repository.c
@@ -113,6 +113,8 @@ out:
void repo_set_worktree(struct repository *repo, const char *path)
{
repo->worktree = real_pathdup(path, 1);
+
+ trace2_worktree(repo->worktree);
}
static int read_and_verify_repository_format(struct repository_format *format,
diff --git a/run-command.c b/run-command.c
index 84b883c213..e833d9a277 100644
--- a/run-command.c
+++ b/run-command.c
@@ -706,6 +706,7 @@ fail_pipe:
cmd->err = fderr[0];
}
+ trace2_child_start(cmd);
trace_run_command(cmd);
fflush(NULL);
@@ -911,6 +912,8 @@ fail_pipe:
#endif
if (cmd->pid < 0) {
+ trace2_child_exit(cmd, -1);
+
if (need_in)
close_pair(fdin);
else if (cmd->in)
@@ -949,13 +952,16 @@ fail_pipe:
int finish_command(struct child_process *cmd)
{
int ret = wait_or_whine(cmd->pid, cmd->argv[0], 0);
+ trace2_child_exit(cmd, ret);
child_process_clear(cmd);
return ret;
}
int finish_command_in_signal(struct child_process *cmd)
{
- return wait_or_whine(cmd->pid, cmd->argv[0], 1);
+ int ret = wait_or_whine(cmd->pid, cmd->argv[0], 1);
+ trace2_child_exit(cmd, ret);
+ return ret;
}
diff --git a/run-command.h b/run-command.h
index 3932420ec8..a91206b08c 100644
--- a/run-command.h
+++ b/run-command.h
@@ -12,6 +12,11 @@ struct child_process {
struct argv_array args;
struct argv_array env_array;
pid_t pid;
+
+ int trace2_child_id;
+ uint64_t trace2_child_us_start;
+ const char *trace2_child_class;
+
/*
* Using .in, .out, .err:
* - Specify 0 for no redirections (child inherits stdin, stdout,
diff --git a/usage.c b/usage.c
index cc803336bd..1838c46d20 100644
--- a/usage.c
+++ b/usage.c
@@ -28,12 +28,17 @@ static NORETURN void usage_builtin(const
Re: [PATCH v4 4/6] push doc: correct lies about how push refspecs work
On Fri, Aug 31, 2018 at 6:24 PM Junio C Hamano wrote: > > Ævar Arnfjörð Bjarmason writes: > > > On Thu, Aug 30 2018, Ævar Arnfjörð Bjarmason wrote: > > > > [Notes to self] > > ... > > > > Later below this we say: > > > > Pushing an empty allows you to delete the ref from the > > remote repository. > > > > Which, perhaps given the discussion of deletions as updates, should be > > mentioned earlier in some way, i.e. should we just say above all these > > rules that by "update" we mean non-deletions? > > You raised good points. The rule that applies to deletion is quite > different from the one for update, we want to make sure readers know > updates and deletions are different. As the rule for deletion is a > lot simpler (i.e. you can always delete unless a configuration or > pre-receive says otherwise), perhaps it would be sufficient to give > the rules for deletion upfront in one section, and then start the > section(s) for update with a phrase like "rules for accepting > updates are follows" after that. Yeah, that was the plan. I'll do that.
Re: [PATCH 2/3] Add test for commit --dry-run --short.
Ævar Arnfjörð Bjarmason writes: > On Fri, Aug 31 2018, Stephen P. Smith wrote: > >> Add test for commit with --dry-run --short for a new file of zero >> length. >> >> The test demonstrated that the setting of the commitable flag was >> broken as was found durning an earlier patch review. >> >> Signed-off-by: Stephen P. Smith >> --- >> t/t7501-commit.sh | 10 ++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh >> index 810d4cea7..fc69da816 100755 >> --- a/t/t7501-commit.sh >> +++ b/t/t7501-commit.sh >> @@ -682,4 +682,14 @@ test_expect_success '--dry-run with conflicts fixed >> from a merge' ' >> git commit -m "conflicts fixed from merge." >> ' >> >> +test_expect_success '--dry-run --short with conflicts fixed from a merge' ' >> +# setup two branches with conflicting information >> +# in the same file, resolve the conflict, >> +# call commit with --dry-run --short >> +rm -f test-file && >> +touch testfile && >> +git add test-file && >> +git commit --dry-run --short >> +' >> + >> test_done > > Ditto my comment on 1/3 on this. I.e. this changes the failing tests in > this series from 2 to 3. Correct. Thanks for helping Stephen on this topic.
Re: [PATCH v4 4/6] push doc: correct lies about how push refspecs work
Ævar Arnfjörð Bjarmason writes: > On Thu, Aug 30 2018, Ævar Arnfjörð Bjarmason wrote: > > [Notes to self] > ... > > Later below this we say: > > Pushing an empty allows you to delete the ref from the > remote repository. > > Which, perhaps given the discussion of deletions as updates, should be > mentioned earlier in some way, i.e. should we just say above all these > rules that by "update" we mean non-deletions? You raised good points. The rule that applies to deletion is quite different from the one for update, we want to make sure readers know updates and deletions are different. As the rule for deletion is a lot simpler (i.e. you can always delete unless a configuration or pre-receive says otherwise), perhaps it would be sufficient to give the rules for deletion upfront in one section, and then start the section(s) for update with a phrase like "rules for accepting updates are follows" after that.
Re: [PATCH 2/5] t5303: test some corrupt deltas
Johannes Schindelin writes: >> Would "echo base >base" give us 5-byte long base even on Windows? > > Please note that Unix shell scripting is a foreign thing on Windows. As > such, there is not really any "native" shell we can use [*1*], and Yeah, I know that; otherwise I wouldn't have asked. Because ... > therefore we use MSYS2's Bash which outputs Unix line endings. ... I didn't know what MSYS folks chose, and/or if you have chosen to tweak their choice, and/or if you switched to somebody else's shell (e.g. busybox) and/or you chose to tweak what they do out of the box, it was worth asking and getting yes/no question. You do not have to tell me why I should be asking. So instead of typing 3 lines, you can just say "yes we use echo that emulates Unix". Thanks.
Re: [PATCH 5/6] pack-bitmap: save "have" bitmap from walk
On Tue, Aug 21 2018, Jeff King wrote:
> +int bitmap_has_sha1_in_uninteresting(struct bitmap_index *bitmap_git,
> + const unsigned char *sha1)
> +{
> + int pos;
> +
> + if (!bitmap_git)
> + return 0; /* no bitmap loaded */
> + if (!bitmap_git->result)
> + BUG("failed to perform bitmap walk before querying");
Some part of what calls this completely breaks pushing from the "next"
branch when you have local bitmaps (we *really* should have some tests
for this...).
I don't have time to dig now, but just on my local git.git on next @
b1634b371dc2e46f9b43c45fd1857c2e2688f96e:
u git (next $=) $ git repack -Ad --window=10 --depth=10
--write-bitmap-index --pack-kept-objects
Enumerating objects: 616909, done.
Counting objects: 100% (616909/616909), done.
Delta compression using up to 8 threads
Compressing objects: 100% (146475/146475), done.
Writing objects: 100% (616909/616909), done.
Selecting bitmap commits: 168027, done.
BBuilding bitmaps: 100%
(338/338), done.
Total 616909 (delta 497494), reused 582609 (delta 467530)
u git (next $=) $ ./git --exec-path=$PWD push avar next:avar/next-push
Enumerating objects: 1330, done.
BUG: pack-bitmap.c:1132: failed to perform bitmap walk before querying
error: pack-objects died of signal 6
error: pack-objects died of signal 6
error: remote unpack failed: eof before pack header was fully read
error: failed to push some refs to 'g...@github.com:avar/git.git'
Removing the bitmap makes it work again:
u git (next $=) $ rm
.git/objects/pack/pack-496088d9464cd79dfcac50dd0d72dcd6faee8253.bitmap
rm: remove write-protected regular file
'.git/objects/pack/pack-496088d9464cd79dfcac50dd0d72dcd6faee8253.bitmap'? y
u git (next $=) $ ./git --exec-path=$PWD push avar next:avar/next-push
Enumerating objects: 2834, done.
Counting objects: 100% (1799/1799), done.
Delta compression using up to 8 threads
Compressing objects: 100% (591/591), done.
Writing objects: 100% (1390/1390), 430.11 KiB | 15.36 MiB/s, done.
Total 1390 (delta 1100), reused 1072 (delta 798)
remote: Resolving deltas: 100% (1100/1100), completed with 214 local
objects.
To github.com:avar/git.git
* [new branch]next -> avar/next-push
Today I deployed next + my patches @ work. Broke pushes in one place
where repacks were being done manually with --write-bitmap-index.
Re: Feature request: hooks directory
On Fri, Aug 31 2018, Wesley Schwengle wrote: > Hop, > > 2018-08-30 16:45 GMT+02:00 Ævar Arnfjörð Bjarmason : > >>> Solution: >>> We discussed this at work and we thought about making a .d directory >>> for the hooks, eg. $GIT_DIR/hooks/post-commit.d, where a user can put >>> the post-commit hooks in. This allows us to provide post commit hooks >>> and allows the user to add additional hooks him/herself. We could >>> implement this in our own code base. But we were wondering if this >>> approach could be shared with the git community and if this behavior >>> is wanted in git itself. >> >> There is interest in this. This E-Mail of mine gives a good summary of >> prior discussions about this: >> https://public-inbox.org/git/877eqqnq22@evledraar.gmail.com/ >> >> I.e. it's something I've personally been interested in doing in the >> past, there's various bolt-on solutions to do it (basically local hook >> runners) used by various projects. > > Thank you for the input. Do you by any chance still have that branch? > Or would you advise me to start fresh, if so, do you have any pointers > on where to look as I'm brand new to the git source code? No, sorry. Start by grepping the hook names found in the githooks manpage in the C code. One of the things that's hard, well not hard, just tedious about this, is that most of them are implementing their own ad-hoc way of doing stuff. E.g. the *-receive hooks are in receive-pack.c in run_receive_hook(). There is run_hook_le() and friends, but it's not used by everything. So e.g. for the pre-receive hook in order to run 2 of them instead of 1 you need to untangle the state where we're feeding the hook with the input (and potentially buffer it, not stream it), instead of doing it as a one-off as we're doing now. Then some hooks get nothing on stdin, some get stuff on stdin, some produce output on stdout/stderr etc. As a first approximation, just add a e.g. support for a pre-receive.2 hook, that gets run after pre-receive, to see what needs to be done to run it twice. > From the thread I've extracted three stories: > > 1) As a developer I want to have 'hooks.multiHooks' to enable > multi-hook support in git > Input is welcome for another name. Yeah maybe we should have a setting, but FWIW I think we should just stat() whether the hooks/$hook_name.d directory exist, and then use it, although maybe we'll need stuff like hooks.multiHooks to give the likes of GitLab (which already do that themselves) an upgrade path... You can see their implementation here: https://gitlab.com/gitlab-org/gitlab-shell/blob/master/lib/gitlab_custom_hook.rb > 2) As a developer I want natural sort order executing for my githooks > so I can predict executions > See > https://public-inbox.org/git/CACBZZX6AYBYeb5S4nEBhYbx1r=icJ81JGYBx5=h4wacphhj...@mail.gmail.com/ > for more information Yeah, any sane implementation of this will execute the hooks in hooks/$hook_name.d in glob() order. > 3) As a developer I want to run $GIT_DIR/hooks/ before > $GIT_DIR/hooks/.d/* > Reference: > https://public-inbox.org/git/cacbzzx6j6q2dun_z-pnent1u714dvnpfbrl_pieqylmczlu...@mail.gmail.com/ For e.g. GitLab the hook/pre-receive is a runner that'll run all hook/pre-receive.d/*, so this probably makes sense to hide behind a config setting. I think it's sensible as a default to move to to just try to move away from hooks/ and use hook/.d/* instead. > The following story would be.. nice to have I think. I'm not sure I > would want to implement this from the get go as I don't have a use > case for it. > 4) As a developer I want a way to have a hook report an error and let > another hook decide if we want to pass or not. > Reference: > https://public-inbox.org/git/xmqq60v4don1@gitster.mtv.corp.google.com/ I think a default that makes more sense is a while ! ret = glob() loop, i.e. a failure will do early exit. But yeah. Junio seemed to want this to be configurable. > 2018-08-31 5:16 GMT+02:00 Jonathan Nieder : >> A few unrelated thoughts, to expand on this. >> >> Separately from that, in [1] I mentioned that I want to revamp how >> hooks work somewhat, to avoid the attack described there (or the more >> common attack also described there that involves a zip file). Such a >> revamp would be likely to also handle this multiple-hook use case. >> >> [1] >> https://public-inbox.org/git/20171002234517.gv19...@aiede.mtv.corp.google.com/ > > The zip file attack vector doesn't change with adding a hook.d > directory structure? If I have one file or multiple files, the attack > stays the same? > I think I'm asking if this would be a show stopper for the feature. Yeah I don't see how what Jonathan's talking about there has any relevance to whether we run 1 or 100 hooks.
Re: Automatic core.autocrlf?
On Fri, Aug 31, 2018 at 07:57:04AM -0400, Randall S. Becker wrote: > > On August 30, 2018 11:29 PM, Jonathan Nieder wrote: > > Torsten Bögershausen wrote: > > > > > My original plan was to try to "make obsolete"/retire and phase out > > > core.autocrlf completely. > > > However, since e.g. egit/jgit uses it > > > (they don't have support for .gitattributes at all) I am not sure if > > > this is a good idea either. > > > > Interesting. [1] appears to have intended to add this feature. > > Do you remember when it is that you tested? > > > > Feel free to file bugs using [2] for any missing features. > > > > [1] https://git.eclipse.org/r/c/60635 > > [2] https://www.eclipse.org/jgit/support/ Oh, (I thought that I digged into the source recently...) Thanks for the clarification > > My testing was done on EGit 5.0.1 yesterday, which does not provide a default > to core.autocrlf. > > Cheers, > Randall > OK, then I assume that jgit supports .gitattributes now. Just to ask a possibly stupid question: did anybody test it ?
Re: Feature request: hooks directory
Hop, 2018-08-30 16:45 GMT+02:00 Ævar Arnfjörð Bjarmason : >> Solution: >> We discussed this at work and we thought about making a .d directory >> for the hooks, eg. $GIT_DIR/hooks/post-commit.d, where a user can put >> the post-commit hooks in. This allows us to provide post commit hooks >> and allows the user to add additional hooks him/herself. We could >> implement this in our own code base. But we were wondering if this >> approach could be shared with the git community and if this behavior >> is wanted in git itself. > > There is interest in this. This E-Mail of mine gives a good summary of > prior discussions about this: > https://public-inbox.org/git/877eqqnq22@evledraar.gmail.com/ > > I.e. it's something I've personally been interested in doing in the > past, there's various bolt-on solutions to do it (basically local hook > runners) used by various projects. Thank you for the input. Do you by any chance still have that branch? Or would you advise me to start fresh, if so, do you have any pointers on where to look as I'm brand new to the git source code? >From the thread I've extracted three stories: 1) As a developer I want to have 'hooks.multiHooks' to enable multi-hook support in git Input is welcome for another name. 2) As a developer I want natural sort order executing for my githooks so I can predict executions See https://public-inbox.org/git/CACBZZX6AYBYeb5S4nEBhYbx1r=icJ81JGYBx5=h4wacphhj...@mail.gmail.com/ for more information 3) As a developer I want to run $GIT_DIR/hooks/ before $GIT_DIR/hooks/.d/* Reference: https://public-inbox.org/git/cacbzzx6j6q2dun_z-pnent1u714dvnpfbrl_pieqylmczlu...@mail.gmail.com/ The following story would be.. nice to have I think. I'm not sure I would want to implement this from the get go as I don't have a use case for it. 4) As a developer I want a way to have a hook report an error and let another hook decide if we want to pass or not. Reference: https://public-inbox.org/git/xmqq60v4don1@gitster.mtv.corp.google.com/ 2018-08-31 5:16 GMT+02:00 Jonathan Nieder : > A few unrelated thoughts, to expand on this. > > Separately from that, in [1] I mentioned that I want to revamp how > hooks work somewhat, to avoid the attack described there (or the more > common attack also described there that involves a zip file). Such a > revamp would be likely to also handle this multiple-hook use case. > > [1] > https://public-inbox.org/git/20171002234517.gv19...@aiede.mtv.corp.google.com/ The zip file attack vector doesn't change with adding a hook.d directory structure? If I have one file or multiple files, the attack stays the same? I think I'm asking if this would be a show stopper for the feature. Cheers, Wesley -- Wesley Schwengle, Developer Mintlab B.V., https://www.zaaksysteem.nl E: wes...@mintlab.nl T: +31 20 737 00 05
RE: Automatic core.autocrlf?
On August 30, 2018 11:29 PM, Jonathan Nieder wrote: > Torsten Bögershausen wrote: > > > My original plan was to try to "make obsolete"/retire and phase out > > core.autocrlf completely. > > However, since e.g. egit/jgit uses it > > (they don't have support for .gitattributes at all) I am not sure if > > this is a good idea either. > > Interesting. [1] appears to have intended to add this feature. > Do you remember when it is that you tested? > > Feel free to file bugs using [2] for any missing features. > > [1] https://git.eclipse.org/r/c/60635 > [2] https://www.eclipse.org/jgit/support/ My testing was done on EGit 5.0.1 yesterday, which does not provide a default to core.autocrlf. Cheers, Randall
Re: [PATCH] config.mak.uname: resolve FreeBSD iconv-related compilation warning
On Fri, Aug 31, 2018 at 11:52 AM Eric Sunshine wrote: > OLD_ICONV has long been needed by FreeBSD so config.mak.uname defines > it unconditionally. However, recent versions do not need it, and its > presence results in compilation warnings. Resolve this issue by defining > OLD_ICONV only for older FreeBSD versions. This seems sane, but just for context is FreeBSD ports itself just compiling without iconv entirely? [CC FreeBSD devel/git maintainer] $ uname -r; grep -ri iconv /usr/ports/devel/git 11.2-RELEASE-p2 /usr/ports/devel/git/Makefile:OPTIONS_DEFINE= GUI SVN GITWEB CONTRIB P4 CVS HTMLDOCS PERL ICONV CURL \ /usr/ports/devel/git/Makefile:OPTIONS_DEFAULT= CONTRIB P4 CVS PERL GITWEB ICONV CURL SEND_EMAIL PCRE \ /usr/ports/devel/git/Makefile:ICONV_USES= iconv /usr/ports/devel/git/Makefile:ICONV_MAKE_ARGS_OFF= NO_ICONV=1 I have little clue about how ports works, but just noticed that they're not monkeypatching in OLD_ICONV there.
Re: Thank you for public-inbox!
Jonathan Nieder wrote: > Eric Wong wrote: > > Jonathan Nieder wrote: > >> Jeff King wrote: > > >>> I guess I just wonder if I set up a mirror on another domain, would > >>> anybody actually _use_ it? I'd think most people would just go to > >>> public-inbox.org as the de facto URL. > >> > >> If it's faster than public-inbox.org and you don't mind the traffic I > >> would send, then I'll use it. :) > > > > Is performance a problem on public-inbox.org for you? > > It's pretty fast. I'm just very, very picky about latency. ;-) Best way for good latency is to have a local mirror, but I guess Googlers still aren't allowed to run AGPL software? > It's good to know you're interested in which corner cases are bad. > The next time I have a noticeably slow page load, I'll contact meta@. Alright. It could also be a general datacenter/networking problem so https://status.linode.com/ (my VPS provider) is worth checking. > [...] > > I've also been sorta considering downgrading to a $5/month VPS > > (from a $20/month VPS) to force myself to pay more attention to > > performance while saving myself a few bucks. But I wouldn't get > > to dogfood on SMP, anymore... > > Sounds reasonable to me. If performance gets bad, that's just a > reason for people to help out (either with patches or e.g. with > donated VMs for hosting). > Speaking of the latter: what are your current resource requirements? Not too much; but could always be better on the software side. > E.g. which of the dimensions in [1] do you not fit into? > [1] https://cloud.google.com/free/docs/always-free-usage-limits#compute_name Dunno, I'm not seeing RAM, there. Depending on traffic, it's around 200MB per-public-inbox-httpd worker (2 workers for 2 cores) when there's a traffic surge on from popular sites. Memory usage is the biggest disappointment and only happens when Varnish can't read fast enough. Everything in the PSGI code is is streamed if possible(*). My goal is to maintain <50MB per worker process, but it could be tough in Perl5. Anyways $20/month gets me 4GB RAM (so I have way more than I need). CPU usage isn't even noticeable (only bursts) and I do other stuff on that server all the time. HDDs wouldn't work well at all and I've noticed differences based on SSD quality with Xapian. Storage for Xapian+SQLite is 4-5x what's in git, so for this list, it's under 7G total (but more will be needed for Xapian reindexing/compact and git repacking). (*) Individual messages for returning giant mboxes and threads are all lazily fleshed out from skeleton data structures as the client socket becomes writable (and quickly discarded after writing). Technically it's all compatible with any PSGI server, but all the streaming stuff is tailored to run on public-inbox-httpd. But there's also git-http-backend memory use which comes in bursts (bitmaps enabled, of course)
Re: Git in Outreachy Dec-Mar?
Hi everyone, I was Outreachy intern last winter. I guess I need to speak up: I will be happy if my feedback helps you. At first, I want to repeat all thanks to Outreachy organizers and Git mentors. That was unique experience and I am so proud of being a part of this project. But, I need to say that my internship wasn't ideal. Mentors, please do not feel guilty: I just want to improve the quality of future internships and give some advises. I guess some of the problems aren't related to Git, and it's Outreachy weak points. Please forward this email to Outreachy organizers if you want. 1. The main problem of Outreachy internship is positioning. I mean, I had strong confidence that it's an internship for newbies in programming. All my friends had the same confidence, and that's the reason why 2 my friends failed in the middle of the Outreachy internship. Load was so big for them, noone explained this fact in the beginning, noone helped with this situation during the internship. I was thinking I could be overqualified and I took someone's place (I had 2 other SWE internships before Outreachy). The truth is that my skills were barely enough. 2. Please tell more about minimal requirements: write it down on a landing page in the beginning and maybe repeat them in every task. I guess it would be the same this year: good knowledge of C, gdb, Git (as a user: intern needs to know how to work with forks, git remote, git rebase -i, etc), Shell, base understanding of Linux terminal, being ready to work remotely. It's good idea to mention that it's not 100% requirement, but anyway at least 60% from the list must be familiar. 3. If you decide to be a mentor - at first, thanks a lot. Please be ready to spend A LOT OF time on it. You need to explain not only the task to your intern, but also how to split the task into subtasks, how to look for solutions, how to work with the terminal, how to debug better and many other questions. It's not only about solving internship task. It's about learning something new. And I did not mention code reviews: there would be many stupid errors and it's a talent not to be angry about that. 4. I fully sure that you need to talk with your intern by the voice. I mean regular calls, at least once a week. It's good idea to share the desktop and show how you are working, what are you using, etc. Ask your intern to share the desktop: you need to feel confident that they understand how to work with the task. Help them with the shortcuts. Remote work is so hard at the beginning, I feel alone with all my problems, feel ashamed to ask questions (because they are not "smart enough"), sometimes I didn't know what to ask. I need to mention that I had almost 1 year of remote work experience, and that helped me a lot. But other interns do not have such experience. Actually, I am sure that the only reason why I successfully finished the internship is that my mentors believed in me and did not fire me in the middle. I personally think that I failed first half of the internship, and only in the end I had almost clear understanding what's going on. (My friend was fired in the same situation.) 5. In the ideal world, I want to force all mentors to get special courses (it will solve problems 2-3-4). Great developer is not equal to great mentor. And, if you work with really newbie, it becomes so necessary. I hope that was useful. In the end I want to say that there's no special requirements to involve people from unrepresented groups. I see no racism or sexism in mailing lists, my mentors were polite and friendly, I can't say anything bad here. Please keep this safe environment and explain your colleagues if you see something bad. In my opinion, the problem is that Git is not friendly with newbies in general. We do not have task tracker, regular mentors (without any special programs: just some developers that are ready to help with first patch). The code is not structured properly, this is additional difficulty for newbie. This system with mailing lists and patches... I understand that it's not possible to make all processes perfect in one moment, but at least we need to help all newbies to solve all these problems in the beginning. I guess that there are only 2 scenarios how to become Git developer. First one is internship. Second is to ask your colleague (who is Git developer) to help you. I don't want to speak on behalf of all women, but I guess many girls feel not confident enough to ask for such help. For me the only possibility to start was the internship. Some personal info: I am in the process of changing jobs. I wish I could help you with mentoring (not as a main mentor, maybe as a second or third one - my experience as an intern could be useful, I could help other interns to start), but I can't predict my load. If you are interested in my help, please write me. And, by the way, please delete my task from list of internship tasks, I will finish it by myself just when I have some free time :)
Re: A rebase regression in Git 2.18.0
Hi Elijah, On Thu, 30 Aug 2018, Elijah Newren wrote: > On Tue, Aug 28, 2018 at 9:58 AM Junio C Hamano wrote: > > > > Elijah Newren writes: > > > > > - Add a flag to turn off directory rename detection, and set the > > > flag for every call from am.c in order to avoid problems like this. > > > > I'd say this is the only practical solution, before you deprecate > > the "pipe format-patch output to am -3" style of "git rebase" (and > > optionally replace with something else). > > I posted a patch a while back to add an --am flag to "git rebase", > make "--am" be implied by options which are still am-specific > (--whitespace, --committer-date-is-author-date, and -C), and change > --merge to be the default. Didn't you also post a patch to fold --merge into the --interactive backend? What's your current state of thinking about this? As to switching from --am as the default: I still think that --am has serious speed advantages over --merge (or for that matter, --interactive). I have no numbers to back that up, though, and I am currently really busy with working on the CI, so I won't be able to measure these numbers, either... Also please note: I converted the `am` backend to pure C (it is waiting at https://github.com/gitgitgadget/git/pull/24, to be submitted after the v2.19.0 RC period). Switching to `--merge` as the default would force me to convert that backend, too ;-) > I'll post it as an RFC again after the various rebase-rewrite series > have settled and merged down...along with my other rebase cleanups > that I was waiting on to avoid conflicts with GSoC stuff. Thanks for waiting! Please note that I am interested, yet I will be on vacation for a couple of weeks in September. Don't let that stop you, though! > > The whole point of "am -3" is to do _better_ than just "patch" with > > minimum amount of information available on the pre- and post- image > > blobs, without knowing the remainder of the tree that the patch did > > not touch. It is not surprising that the heuristics that look at > > the unchanging part of the tree to infer renames that may or may not > > exist guesses incorrectly, either with false positive or negative. > > In the context of "rebase", we always have all the trees that are > > involved. We should be able to do better than "am -3". Right. I think that Elijah's right, and --merge is that "do better" solution. Ciao, Dscho
Re: [PATCH 2/5] t5303: test some corrupt deltas
Hi Junio, On Thu, 30 Aug 2018, Junio C Hamano wrote: > Jeff King writes: > > > +test_expect_success \ > > +'apply delta with too many copied bytes' \ > > +'printf "\5\1\221\0\2" > too_big_copy && > > + echo base >base && > > + test_must_fail test-tool delta -p base too_big_copy /dev/null' > > Would "echo base >base" give us 5-byte long base even on Windows? Please note that Unix shell scripting is a foreign thing on Windows. As such, there is not really any "native" shell we can use [*1*], and therefore we use MSYS2's Bash which outputs Unix line endings. Ciao, Dscho Footnote *1*: I keep trying (and failing) to find the time to work on the pure-Win32 port of BusyBox, which would give us "sort of a native Unix shell". That probably *would* output CR/LF in this case. But as there are still parts of the test suite that require Perl (which is not included in BusyBox), I think we are still a lng way from running the test suite in a pure Win32 fashion. With all the time tax that incurs for us Windows users.
Re: improved diff tool
On Thu, Aug 30 2018, Piers Titus van der Torren wrote:
> Is there interest to incorporate this algorithm in the main git
> codebase? And if so, any hints on how to proceed?
This looks very nice, it would be great to have it in git. I think it's
more useful to focus on getting it into the git C codebase, the user
base of gitk/git-gui is tiny by comparison.
Aside from what others have mentioned, maybe this commit helps to get an
idea of how to do stuff like this: 3cde4e02ee ("diff: retire
"compaction" heuristics", 2016-12-23)
I.e. it's one of our previous diff algorithms being ripped out. Grepping
for the various --diff-algorithm=* options in "man git-diff" in the
source/history should also give good ideas of where to start hooking in.
I see from your name / some brief spying on you via Google that you
might be in the Amsterdam area. I'm not very familiar with the diff part
of the codebase, but if you'd like I'd be happy to meet in person
sometime to help you get started on various other stuff in git.git if
you'd like.
Re: improved diff tool
On Thu, Aug 30, 2018 at 7:22 PM, Stefan Beller wrote: > > AFAICT this is more than just a coloring scheme as it both produces > a different low level diff, which would need code in the xdiff parts > as well as colors, that is in diff.c. About the low level diff, Michael Haggerty's tools for experimenting with diff "slider" heuristics might also help: https://github.com/mhagger/diff-slider-tools
Re: Git in Outreachy Dec-Mar?
On Thu, Aug 30, 2018 at 9:24 PM, Jeff King wrote: > On Thu, Aug 30, 2018 at 01:46:00PM +0200, Johannes Schindelin wrote: > >> I am willing to mentor, and the only reason that kept me from already >> stepping forward and trying to brush up the landing page is this concern: >> traditionally, we (as in: the core Git contributors) have been less than >> successful in attracting and retaining contributors from under-represented >> groups. I don't think any regular reader of this mailing list can deny >> that. >> >> And while I find it very important to reach out (there are just *so* many >> benefits to having a more diverse team), I have to ask *why* we are so >> unsuccessful. As long as we do not even know the answer to that, is it >> even worth pursuing Outreachy? >> >> I mean, if we make serious mistakes here, without even realizing, that >> directly lead to being stuck in our old bubble, then we are prone to >> simply repeat those mistakes over and over and over again. And that would >> just be a waste of our time, *and* a big de-motivator for the Outreachy >> students. >> >> What's your take on this? > > My feeling is that our lack of diversity has less to do with driving out > diverse candidates, and more that they do not join in the first place. I agree with that. > Which isn't to say we _wouldn't_ drive out diversity, but that I'm not > sure we have very good data on what happens in that second stage. Maybe we could ask Olga in CC what we could do better? > If we > can use the program to overcome "step 1", that helps us get that data > (and hopefully react to it in time to be useful, and not just use the > candidate as a guinea pig; I agree there is the possibility of doing > more harm than good to a student who becomes de-motivated). I agree. > That leaves aside the question of whether things we are doing prevent > people from participating in the first place. I'm certainly open to that > idea, but I think it's a separate discussion. Yeah, I think there is a lot we could do to improve in this area and it would help.
[PATCH] config.mak.uname: resolve FreeBSD iconv-related compilation warning
From: Jonathan Nieder OLD_ICONV has long been needed by FreeBSD so config.mak.uname defines it unconditionally. However, recent versions do not need it, and its presence results in compilation warnings. Resolve this issue by defining OLD_ICONV only for older FreeBSD versions. Specifically, revision r281550[1], which is part of FreeBSD 11, removed the need for OLD_ICONV, and r282275[2] back-ported that change to 10.2. Versions prior to 10.2 do need it. [1] https://github.com/freebsd/freebsd/commit/b0813ee288f64f677a2cebf7815754b027a8215b [2] https://github.com/freebsd/freebsd/commit/b709ec868adb5170d09bc5a66b18d0e0d5987ab6 [es: commit message; tweak version check to distinguish 10.x versions] Signed-off-by: Jonathan Nieder Signed-off-by: Eric Sunshine --- This is a follow-up to [1] which encapsulates Jonathan's proposed change as a proper patch. I made Jonathan as the author since he did all the hard research and formulated the core of the change (whereas I only reported the issue and extended the version check to correctly handle FreeBSD 10.0 and 10.1). Jonathan's sign-off comes from [1]. [1]: https://public-inbox.org/git/20180805075736.gf44...@aiede.svl.corp.google.com/ config.mak.uname | 12 +++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/config.mak.uname b/config.mak.uname index 2be2f19811..e47af72e01 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -192,7 +192,17 @@ ifeq ($(uname_O),Cygwin) endif ifeq ($(uname_S),FreeBSD) NEEDS_LIBICONV = YesPlease - OLD_ICONV = YesPlease + # Versions up to 10.1 require OLD_ICONV; 10.2 and beyond don't. + # A typical version string looks like "10.2-RELEASE". + ifeq ($(shell expr "$(uname_R)" : '[1-9]\.'),2) + OLD_ICONV = YesPlease + endif + ifeq ($(firstword $(subst -, ,$(uname_R))),10.0) + OLD_ICONV = YesPlease + endif + ifeq ($(firstword $(subst -, ,$(uname_R))),10.1) + OLD_ICONV = YesPlease + endif NO_MEMMEM = YesPlease BASIC_CFLAGS += -I/usr/local/include BASIC_LDFLAGS += -L/usr/local/lib -- 2.19.0.rc1.352.gb1634b371d
Re: Git in Outreachy Dec-Mar?
Hi, On Tue, Aug 28, 2018 at 5:14 PM, Jeff King wrote: > The Outreachy application period is set to begin on September 10th for > interns participating in the December-March program. Do we want to > participate? > > Details on the program are here: > > https://www.outreachy.org/communities/cfp/ > > If we want to, then we need: > > 1. Volunteers to mentor. This is similar in scope to being a GSoC > mentor. I volunteer to co-mentor. > 2. To get our landing page and list of projects in order (and also > micro-projects for applicants). This can probably build on the > previous round at: > >https://git.github.io/Outreachy-15/ > > and on the project/microprojects lists for GSoC (which will need > some updating and culling). Ok to take a look at that. > 3. To figure out funding (unlike GSoC, the intern stipend comes from > the projects). I can look into getting outside funds (which is what > we did last year). Worst case, we do have enough project money to > cover an intern. Last year[1] opinions were that this was a > reasonable use of project money, but of course new opinions are > welcome. I can also look at getting outside funds. My opinion though is that it is probably better if the Git project can use its own fund for this, as it makes it easier for possible mentors if they don't need to look at getting outside funds. Thanks for sending this, Christian.
Re: [PATCH 3/3] wt-status.c: Set the commitable flag in the collect phase.
On Fri, Aug 31 2018, Stephen P. Smith wrote:
> In an update to fix a bug with "commit --dry-run" it was found that
> the commitable flag was broken. The update was, at the time,
> accepted as it was better than the previous version.
What update is this? I.e. git.git commit id? See the "or this invocation
of `git show`" part of SubmittingPatches for how to quote it in the
commit message.
> Since the set of the flag had been done in wt_longstatus_print_updated,
> set the flag in wt_status_collect_updated_cb.
>
> Set the commitable flag in wt_status_collect_changes_initial to keep
> from introducing a rebase regression.
>
> Leave the setting of the commitable flag in show_merge_in_progress. If
> a check for merged commits is moved to the collect phase then other
> --dry-run tests fail.
>
> Signed-off-by: Stephen P. Smith
> ---
> wt-status.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/wt-status.c b/wt-status.c
> index 5ffab6101..d50798425 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -540,10 +540,12 @@ static void wt_status_collect_updated_cb(struct
> diff_queue_struct *q,
> /* Leave {mode,oid}_head zero for an add. */
> d->mode_index = p->two->mode;
> oidcpy(>oid_index, >two->oid);
> + s->commitable = 1;
> break;
> case DIFF_STATUS_DELETED:
> d->mode_head = p->one->mode;
> oidcpy(>oid_head, >one->oid);
> + s->commitable = 1;
> /* Leave {mode,oid}_index zero for a delete. */
> break;
>
> @@ -561,6 +563,7 @@ static void wt_status_collect_updated_cb(struct
> diff_queue_struct *q,
> d->mode_index = p->two->mode;
> oidcpy(>oid_head, >one->oid);
> oidcpy(>oid_index, >two->oid);
> + s->commitable = 1;
> break;
> case DIFF_STATUS_UNMERGED:
> d->stagemask = unmerged_mask(p->two->path);
> @@ -665,11 +668,13 @@ static void wt_status_collect_changes_initial(struct
> wt_status *s)
>* code will output the stage values directly and not
> use the
>* values in these fields.
>*/
> + s->commitable = 1;
> } else {
> d->index_status = DIFF_STATUS_ADDED;
> /* Leave {mode,oid}_head zero for adds. */
> d->mode_index = ce->ce_mode;
> oidcpy(>oid_index, >oid);
> + s->commitable = 1;
> }
> }
> }
> @@ -773,7 +778,6 @@ static void wt_longstatus_print_updated(struct wt_status
> *s)
> continue;
> if (!shown_header) {
> wt_longstatus_print_cached_header(s);
> - s->commitable = 1;
> shown_header = 1;
> }
> wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, it);
This looks sensible, but I'm not familiar with the status code.
Structurally, re: my comment on 1/3 and 2/3, it would make sense to make
this a two-part series. In 1/2 you add the test you're adding in 2/3 as
a test_expect_failure test, and in 2/2 (this commit) you tweak all the
"test_expect_failure" that now pass to "test_expect_success".
Re: [PATCH 2/3] Add test for commit --dry-run --short.
On Fri, Aug 31 2018, Stephen P. Smith wrote: > Add test for commit with --dry-run --short for a new file of zero > length. > > The test demonstrated that the setting of the commitable flag was > broken as was found durning an earlier patch review. > > Signed-off-by: Stephen P. Smith > --- > t/t7501-commit.sh | 10 ++ > 1 file changed, 10 insertions(+) > > diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh > index 810d4cea7..fc69da816 100755 > --- a/t/t7501-commit.sh > +++ b/t/t7501-commit.sh > @@ -682,4 +682,14 @@ test_expect_success '--dry-run with conflicts fixed from > a merge' ' > git commit -m "conflicts fixed from merge." > ' > > +test_expect_success '--dry-run --short with conflicts fixed from a merge' ' > + # setup two branches with conflicting information > + # in the same file, resolve the conflict, > + # call commit with --dry-run --short > + rm -f test-file && > + touch testfile && > + git add test-file && > + git commit --dry-run --short > +' > + > test_done Ditto my comment on 1/3 on this. I.e. this changes the failing tests in this series from 2 to 3.
Re: [PATCH 1/3] Change tests from expecting to fail to expecting success.
On Fri, Aug 31 2018, Stephen P. Smith wrote: > diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh > index 4cae92804..810d4cea7 100755 > --- a/t/t7501-commit.sh > +++ b/t/t7501-commit.sh > @@ -99,12 +99,12 @@ test_expect_success '--dry-run with stuff to commit > returns ok' ' > git commit -m next -a --dry-run > ' > > -test_expect_failure '--short with stuff to commit returns ok' ' > +test_expect_success '--short with stuff to commit returns ok' ' > echo bongo bongo bongo >>file && > git commit -m next -a --short > ' > > -test_expect_failure '--porcelain with stuff to commit returns ok' ' > +test_expect_success '--porcelain with stuff to commit returns ok' ' > echo bongo bongo bongo >>file && > git commit -m next -a --porcelain This commit is not OK and needs to be folded into later commits. It makes the test suite fail until (presumably, haven't reviewed the rest) a later commit. The tests must always pass, otherwise someone bisecting will trip up over this commit.
[PATCH 0/3] doc-diff: add "clean" mode & fix portability problem
This series replaces Peff's solo patch[1] which updates "make clean" to remove doc-diff's temporary directory. Rather than imbuing the Makefile with knowledge specific to doc-diff's internals, this series adds a "clean" mode to doc-diff which removes its temporary worktree and generated files, and has "make clean" invoke that instead. It also fixes a portability problem which prevented doc-diff from working on MacOS and FreeBSD. [1]: https://public-inbox.org/git/20180830195546.ga22...@sigill.intra.peff.net/ Eric Sunshine (3): doc-diff: fix non-portable 'man' invocation doc-diff: add --clean mode to remove temporary working gunk doc/Makefile: drop doc-diff worktree and temporary files on "make clean" Documentation/Makefile | 1 + Documentation/doc-diff | 21 + 2 files changed, 18 insertions(+), 4 deletions(-) -- 2.19.0.rc1.352.gb1634b371d
[PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on "make clean"
doc-diff creates a temporary working tree (git-worktree) and generates a bunch of temporary files which it does not remove since they act as a cache to speed up subsequent runs. Although doc-diff's working tree and generated files are not strictly build products of the Makefile (which, itself, never runs doc-diff), as a convenience, update "make clean" to clean up doc-diff's working tree and generated files along with other development detritus normally removed by "make clean". Signed-off-by: Eric Sunshine --- Documentation/Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/Makefile b/Documentation/Makefile index a42dcfc745..26e268ae8d 100644 --- a/Documentation/Makefile +++ b/Documentation/Makefile @@ -332,6 +332,7 @@ clean: $(RM) SubmittingPatches.txt $(RM) $(cmds_txt) $(mergetools_txt) *.made $(RM) manpage-base-url.xsl + '$(SHELL_PATH_SQ)' ./doc-diff --clean $(MAN_HTML): %.html : %.txt asciidoc.conf $(QUIET_ASCIIDOC)$(RM) $@+ $@ && \ -- 2.19.0.rc1.352.gb1634b371d
[PATCH 1/3] doc-diff: fix non-portable 'man' invocation
doc-diff invokes 'man' with the -l option to force "local" mode,
however, neither MacOS nor FreeBSD recognize this option. On those
platforms, if the argument to 'man' contains a slash, it is
automatically interpreted as a file specification, so a "local"-like
mode is not needed. And, it turns out, 'man' which does support -l
falls back to enabling -l automatically if it can't otherwise find a
manual entry corresponding to the argument. Since doc-diff always
passes an absolute path of the nroff source file to 'man', the -l
option kicks in anyhow, despite not being specified explicitly.
Therefore, make the invocation portable to the various platforms by
simply dropping -l.
Signed-off-by: Eric Sunshine
---
Documentation/doc-diff | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/doc-diff b/Documentation/doc-diff
index f483fe427c..c2906eac5e 100755
--- a/Documentation/doc-diff
+++ b/Documentation/doc-diff
@@ -69,7 +69,7 @@ generate_render_makefile () {
printf '%s: %s\n' "$dst" "$src"
printf '\t@echo >&2 " RENDER $(notdir $@)" && \\\n'
printf '\tmkdir -p $(dir $@) && \\\n'
- printf '\tMANWIDTH=80 man -l $< >$@+ && \\\n'
+ printf '\tMANWIDTH=80 man $< >$@+ && \\\n'
printf '\tmv $@+ $@\n'
done
}
--
2.19.0.rc1.352.gb1634b371d
[PATCH 2/3] doc-diff: add --clean mode to remove temporary working gunk
As part of its operation, doc-diff creates a bunch of temporary working files and holds onto them in order to speed up subsequent invocations. These files are never deleted. Moreover, it creates a temporary working tree (via git-wortkree) which likewise never gets removed. Without knowing the implementation details of the tool, a user may not know how to clean up manually afterward. Worse, the user may find it surprising and alarming to discover a working tree which s/he did not create explicitly. To address these issues, add a --clean mode which removes the temporary working tree and deletes all generated files. Signed-off-by: Eric Sunshine --- Documentation/doc-diff | 19 --- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/Documentation/doc-diff b/Documentation/doc-diff index c2906eac5e..f397fd229b 100755 --- a/Documentation/doc-diff +++ b/Documentation/doc-diff @@ -2,20 +2,25 @@ OPTIONS_SPEC="\ doc-diff [options] [-- ] +doc-diff (-c|--clean) -- j=nparallel argument to pass to make f force rebuild; do not rely on cached results +c,cleancleanup temporary working files " SUBDIRECTORY_OK=1 . "$(git --exec-path)/git-sh-setup" parallel= force= +clean= while test $# -gt 0 do case "$1" in -j) parallel=$2; shift ;; + -c|--clean) + clean=t ;; -f) force=t ;; --) @@ -26,6 +31,17 @@ do shift done +cd_to_toplevel +tmp=Documentation/tmp-doc-diff + +if test -n "$clean" +then + test $# -eq 0 || usage + git worktree remove --force "$tmp/worktree" 2>/dev/null + rm -rf "$tmp" + exit 0 +fi + if test -z "$parallel" then parallel=$(getconf _NPROCESSORS_ONLN 2>/dev/null) @@ -42,9 +58,6 @@ to=$1; shift from_oid=$(git rev-parse --verify "$from") || exit 1 to_oid=$(git rev-parse --verify "$to") || exit 1 -cd_to_toplevel -tmp=Documentation/tmp-doc-diff - if test -n "$force" then rm -rf "$tmp" -- 2.19.0.rc1.352.gb1634b371d
