* Re: [PATCH 1/2] asciidoctor: fix user-manual to be built by `asciidoctor`
From: Junio C Hamano @ 2017-01-07 22:03 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, git, 마누엘
In-Reply-To: <20170104080852.bmlmtzxhjx4qt74f@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Jan 02, 2017 at 05:03:57PM +0100, Johannes Schindelin wrote:
>
>> From: =?UTF-8?q?=EB=A7=88=EB=88=84=EC=97=98?= <nalla@hamal.uberspace.de>
>>
>> The `user-manual.txt` is designed as a `book` but the `Makefile` wants
>> to build it as an `article`. This seems to be a problem when building
>> the documentation with `asciidoctor`. Furthermore the parts *Git
>> Glossary* and *Appendix B* had no subsections which is not allowed when
>> building with `asciidoctor`. So lets add a *dummy* section.
>
> The git-scm.com site uses asciidoctor, too, and I think I have seen some
> oddness with the rendering though. So in general I am in favor of making
> things work under both asciidoc and asciidoctor.
>
> I diffed the results of "make user-manual.html" before and after this
> patch. A lot of "h3" chapter titles get bumped to "h2", which is OK. The
> chapter titles now say "Chapter 1. Repositories and Branches" rather
> than just "Repositories and Branches". Likewise, references now say
>
> Chapter 1, _Repositories and Branches_
>
> rather than:
>
> the section called "Repositories and Branches".
>
> which is probably OK, though the whole thing is short enough that
> calling the sections chapters feels a bit over-important.
Is that a longer way to say that the claim "... is designed as a
book" is false?
> So I dunno. I really do think "article" is conceptually the most
> appropriate style, but I agree that there are some book-like things
> (like appendices).
... Yeah, I should have read forward first before starting to insert
my comments.
^ permalink raw reply
* Re: [PATCH 1/2] asciidoctor: fix user-manual to be built by `asciidoctor`
From: Junio C Hamano @ 2017-01-07 22:00 UTC (permalink / raw)
To: Jeff King
Cc: Lars Schneider, Johannes Schindelin, git,
마누엘
In-Reply-To: <20170105164556.b3bzeqqzx4pvni4z@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> As far as CI goes, I am not altogether convinced of the usefulness of
> building the documentation. It's very expensive, and the failure mode is
> rarely "whoops, running `make doc` failed". It's almost always that the
> output looks subtly wrong, but that's very hard to check automatically.
I've seen "make doc" break for me when it works for other people,
and if CI can check the formatting for various combinations of
versions of toolchain components, it would have some value. But
otherwise, I agree that "make doc" in CI would not give us as much
benefit as we spend electricity on it.
If we are to make support for building with AsciiDoctor better, CI
that does "make doc" with both AsciiDoc and AsciiDoctor might be of
help---a "make it buildable with A" patch may accidentally break B
and vice versa.
> [1] I think we've also traditionally considered asciidoc to be the
> definitive toolchain, and people using asciidoctor are free to
> submit patches to make things work correctly in both places. I'm not
> opposed to changing that attitude, as it seems like asciidoctor is
> faster and more actively maintained these days. But I suspect our
> build chain would need some improvements. Last time I tried building
> with AsciiDoctor it involved a lot manual tweaking of Makefile
> variables. It sounds like Dscho is doing it regularly, though. It
> should probably work out of the box (with something like
> USE_ASCIIDOCTOR=Yes) if we expect people to actually rely on it.
I do not mind getting convinced to declare that we will swap the
positions of these two documentation toolchains in some future date,
and a good first step for it to happen would be an easier out-of-box
experience.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 05/34] sequencer (rebase -i): learn about the 'verbose' mode
From: Junio C Hamano @ 2017-01-07 21:48 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <alpine.DEB.2.20.1701021610590.3469@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi Junio,
>
> On Tue, 13 Dec 2016, Junio C Hamano wrote:
>
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>>
>> > @@ -1493,9 +1498,26 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
>> > }
>> >
>> > if (is_rebase_i(opts)) {
>> > + struct strbuf buf = STRBUF_INIT;
>> > +
>> > /* Stopped in the middle, as planned? */
>> > if (todo_list->current < todo_list->nr)
>> > return 0;
>> > +
>> > + if (opts->verbose) {
>> > + const char *argv[] = {
>> > + "diff-tree", "--stat", NULL, NULL
>> > + };
>> > +
>> > + if (!read_oneliner(&buf, rebase_path_orig_head(), 0))
>> > + return error(_("could not read '%s'"),
>> > + rebase_path_orig_head());
>> > + strbuf_addstr(&buf, "..HEAD");
>> > + argv[2] = buf.buf;
>> > + run_command_v_opt(argv, RUN_GIT_CMD);
>> > + strbuf_reset(&buf);
>> > + }
>> > + strbuf_release(&buf);
>> > }
>>
>> It's a bit curious that the previous step avoided running a separate
>> process and instead did "diff-tree -p" all in C, but this one does not.
>
> I guess my only defence is that I tried to be a little lazy.
I actually was alluding to going the other way around, spawning
"diff-tree -p" in the other codepath like this one does.
^ permalink raw reply
* Re: "git fsck" not detecting garbage at the end of blob object files...
From: Dennis Kaarsemaker @ 2017-01-07 21:47 UTC (permalink / raw)
To: John Szakmeister, git
In-Reply-To: <CAEBDL5Uc39JagdmXUxfxh1TPSK3H5wxoTfjK-pfLRYjciBnHpA@mail.gmail.com>
On Sat, 2017-01-07 at 07:50 -0500, John Szakmeister wrote:
> I was perusing StackOverflow this morning and ran across this
> question: http://stackoverflow.com/questions/41521143/git-fsck-full-only-checking-directories/
>
> It was a simple question about why "checking objects" was not
> appearing, but in it was another issue. The user purposefully
> corrupted a blob object file to see if `git fsck` would catch it by
> tacking extra data on at the end. `git fsck` happily said everything
> was okay, but when I played with things locally I found out that `git
> gc` does not like that extra garbage. I'm not sure what the trade-off
> needs to be here, but my expectation is that if `git fsck` says
> everything is okay, then all operations using that object (file)
> should work too.
>
> Is that unreasonable? What would be the impact of fixing this issue?
If you do this with a commit object or tree object, fsck does complain.
I think it's sensible to do so for blob objects as well.
Editing blob object:
hurricane:/tmp/moo (master)$ hexer .git/objects/a1/b3ebb97f10ff8d85a9472bcba50cb575dbd485
hurricane:/tmp/moo (master)$ git status
On branch master
nothing to commit, working tree clean
hurricane:/tmp/moo (master)$ git fsck
Checking object directories: 100% (256/256), done.
hurricane:/tmp/moo (master)$ git gc
Counting objects: 3, done.
error: garbage at end of loose object 'a1b3ebb97f10ff8d85a9472bcba50cb575dbd485'
fatal: loose object a1b3ebb97f10ff8d85a9472bcba50cb575dbd485 (stored in .git/objects/a1/b3ebb97f10ff8d85a9472bcba50cb575dbd485) is corrupt
error: failed to run repack
Editing tree object:
hurricane:/tmp/moo (master)$ hexer .git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20
hurricane:/tmp/moo (master +)$ git status
error: garbage at end of loose object 'd4eda486f02e3e862e23f6eb3739a25a2ca43f20'
fatal: loose object d4eda486f02e3e862e23f6eb3739a25a2ca43f20 (stored in .git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20) is corrupt
error: garbage at end of loose object 'd4eda486f02e3e862e23f6eb3739a25a2ca43f20'
fatal: loose object d4eda486f02e3e862e23f6eb3739a25a2ca43f20 (stored in .git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20) is corrupt
hurricane:/tmp/moo (master +)$ git fsck
error: garbage at end of loose object 'd4eda486f02e3e862e23f6eb3739a25a2ca43f20'
fatal: loose object d4eda486f02e3e862e23f6eb3739a25a2ca43f20 (stored in .git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20) is corrupt
error: garbage at end of loose object 'd4eda486f02e3e862e23f6eb3739a25a2ca43f20'
fatal: loose object d4eda486f02e3e862e23f6eb3739a25a2ca43f20 (stored in .git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20) is corrupt
Editing commit object:
hurricane:/tmp/moo (master)$ echo test >> .git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0
hurricane:/tmp/moo (master +)$ git status
error: garbage at end of loose object '4759a693f7e8362c724d3365fe6df398083fafa0'
fatal: loose object 4759a693f7e8362c724d3365fe6df398083fafa0 (stored in .git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0) is corrupt
error: garbage at end of loose object '4759a693f7e8362c724d3365fe6df398083fafa0'
fatal: loose object 4759a693f7e8362c724d3365fe6df398083fafa0 (stored in .git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0) is corrupt
!(128) hurricane:/tmp/moo (master +)$ git fsck
error: garbage at end of loose object '4759a693f7e8362c724d3365fe6df398083fafa0'
fatal: loose object 4759a693f7e8362c724d3365fe6df398083fafa0 (stored in .git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0) is corrupt
error: garbage at end of loose object '4759a693f7e8362c724d3365fe6df398083fafa0'
fatal: loose object 4759a693f7e8362c724d3365fe6df398083fafa0 (stored in .git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0) is corrupt
D.
^ permalink raw reply
* Re: [PATCH v3 14/21] read-cache: touch shared index files when used
From: Junio C Hamano @ 2017-01-07 21:46 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <CAP8UFD1EgOxcPi=tpiosKkYMcCZe+b6gwW0CKt2sE1NZ7gQv=A@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> So what should we do if freshen_file() returns 0 which means that the
> freshening failed?
You tell me ;-) as you are the one who is proposing this feature.
Isn't a failure to freshen it a grave error? We are letting a
base/shared index file that is known to be in-use go stale and
eventually subject for garbage collection, and the user should be
notified in some way before the actual GC happens that renders the
index file unusable?
What is the failure mode after such a premature GC happens? What
does the end-user see? Can you try to (1) split the index (2)
modify bunch of entries (3) remove the base/shared index with /bin/rm
and then see how various Git commands fail? Do they fail gracefully?
I am trying to gauge the seriousness of ignoring such an error here.
^ permalink raw reply
* [PATCH] Makefile: POSIX windres
From: Steven Penny @ 2017-01-07 21:41 UTC (permalink / raw)
To: git; +Cc: Steven Penny
When environment variable POSIXLY_CORRECT is set, the "input -o output" syntax
is not supported.
http://cygwin.com/ml/cygwin/2017-01/msg00036.html
Signed-off-by: Steven Penny <svnpenn@gmail.com>
---
Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index d861bd9..a2a1212 100644
--- a/Makefile
+++ b/Makefile
@@ -1816,7 +1816,7 @@ $(SCRIPT_LIB) : % : %.sh GIT-SCRIPT-DEFINES
git.res: git.rc GIT-VERSION-FILE
$(QUIET_RC)$(RC) \
$(join -DMAJOR= -DMINOR=, $(wordlist 1,2,$(subst -, ,$(subst ., ,$(GIT_VERSION))))) \
- -DGIT_VERSION="\\\"$(GIT_VERSION)\\\"" $< -o $@
+ -DGIT_VERSION="\\\"$(GIT_VERSION)\\\"" -i $< -o $@
# This makes sure we depend on the NO_PERL setting itself.
$(SCRIPT_PERL_GEN): GIT-BUILD-OPTIONS
--
2.8.3
^ permalink raw reply related
* Re: [PATCH v3 08/21] Documentation/git-update-index: talk about core.splitIndex config var
From: Junio C Hamano @ 2017-01-07 21:38 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <CAP8UFD06mxGiZmr4Lwv3M8CedBZaamswzz-Q+mOxuuUFet8KNQ@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> It feels strange that when I do things one way, you suggest another
> way, and the next time in a similar situation when I do things the way
> you suggested previously, then you suggest the way I did it initially
> the first time...
Perhaps because neither is quite satisfactory and I am being forced
to choose between the two unsatifactory designs? In any case, I
mostly am and was pointing out the issues and not saying the other
one is the most preferred solution in these threads.
I think there should just be one authoritative source of the truth,
and I have always thought it should be the bit set in the index file
when the command line option is used, because that was the way the
feature was introduced first and I am superstitious about end-user
inertia. And from that point of view, no matter how you make this
new "config" thing interact with it, it would always give a strange
and unsatifactory end-user experience, at least to me.
Perhaps we should declare that config will be the one and only way
in the future and start deprecating the command line option way.
That will remove the need for two to interact with each other.
I dunno.
^ permalink raw reply
* Re: [PATCH] don't use test_must_fail with grep
From: Junio C Hamano @ 2017-01-07 21:18 UTC (permalink / raw)
To: Pranit Bauva; +Cc: Johannes Sixt, Luke Diamand, Git Users, Stefan Beller
In-Reply-To: <CAFZEwPNbtamFfFy7vYXurpEWBDmRMyPB9+Ep-hm4uZVMREbq5Q@mail.gmail.com>
Pranit Bauva <pranit.bauva@gmail.com> writes:
> Hey Johannes,
>
> On Sun, Jan 1, 2017 at 8:20 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> which makes me wonder: Is the message that we do expect not to occur
>> actually printed on stdout? It sounds much more like an error message, i.e.,
>> text that is printed on stderr. Wouldn't we need this?
>>
>> git p4 commit >actual 2>&1 &&
>> ! grep "git author.*does not match" actual &&
>>
>> -- Hannes
>
> This seems better! Since I am at it, I can remove the traces of pipes
> in an another patch.
>
> Regards,
> Pranit Bauva
I see v3 that has 2>&1 but according to Luke's comment ("the message
comes from cat"), it shouldn't be there? I am behind clearing the
backlog in my mailbox and I could tweak it out from v3 while
queuing, or I may forget about it after looking at other topics ;-)
in which case you may want to send v4 with the fix?
^ permalink raw reply
* Re: RFC: --force-with-lease default behaviour
From: Junio C Hamano @ 2017-01-07 21:04 UTC (permalink / raw)
To: G. Sylvie Davies; +Cc: git
In-Reply-To: <CAAj3zPx4uMXhV7t86Cnn8SgmpXb2SGththYN7sHetOqL_JosMg@mail.gmail.com>
"G. Sylvie Davies" <sylvie@bit-booster.com> writes:
> I wonder if there's anything one could do to help those who type "git
> fetch" and still want to enjoy "--force-with-lease"...
The entire idea behind "force-with-lease" is that you plan to later
force update the tip of a branch at the remote to replace the commit
that used to be at the tip at some point, that you do not want other
people to have their own work on that branch that will be lost by
your later force-pushing, yet you cannot "lock" a branch at the
remote repository remotely because that goes against the distributed
nature of the development. Instead of locking others out, forcing
others to wait and sit idle while you complete the material to be
force-pushed (which may never happen), you base your work on one
state of the remote branch, and make sure the remote branch hasn't
advanced in the meantime (or you redo your work)---the cost of the
extra work due to your planned force-pushing is beared by you, not
by others.
There however is no place in Git where you explicitly declare "this
is where I start working on producing a new commit. That commit
will replace this state and will not fast-forward from it." and
store it locally. The "--force-with-lease" was designed to take
that information from the command line, expecting that the script
that drives it does something like
#!/bin/sh
LEASE=$(git rev-parse --verify @{u})
# do whatever that requires non-fast-forward push
git commit --amend ...
... maybe more ...
# finally push it out
git push --force-with-lease $LEASE ...
Lazy people decided that as long as they promise to themselves that
they are not going to do anything to cause @{u} to move, they can
use it as a lazy-man's approximate. Perhaps that was a misguided
attempt to add convenience.
A possible answer to your wordering may be to deprecate the
defaulting to @{u} and always require the expected commit to be
specified explicitly.
^ permalink raw reply
* Re: Git filesystem case-insensitive to case-sensitive system broken
From: Torsten Bögershausen @ 2017-01-07 13:48 UTC (permalink / raw)
To: Steven Robertson; +Cc: git
In-Reply-To: <CALwf_moJFqCKcc5Q=CC6aSSUAGqXEFDKrJUkdwov8shqBRK=yg@mail.gmail.com>
On Fri, Jan 06, 2017 at 01:56:36PM -0800, Steven Robertson wrote:
> Hello,
>
> I was doing development on a linux box on AWS, when we found a code
> bug that had me switching to running the code on a Mac instead. We
> discovered that we had accidentally named two files the same when
> looked at case-insensitively, which made git commands afterwards
> display the wrong thing. It looked like this (ignoring some things
> that aren't relevant):
>
> $ git status
>
>
> modified: tests/test_system/show_19_L.txt
>
>
> no changes added to commit (use "git add" and/or "git commit -a")
>
> $ git checkout tests/test_system/show_19_L.txt
>
> $ git status
>
>
> modified: tests/test_system/show_19_l.txt
>
>
> no changes added to commit (use "git add" and/or "git commit -a")
>
> $ git checkout tests/test_system/show_19_l.txt
>
> $ git status
>
>
> modified: tests/test_system/show_19_L.txt
>
>
> no changes added to commit (use "git add" and/or "git commit -a")
>
> $ diff tests/test_system/show_19_L.txt tests/test_system/show_19_l.txt
>
> $
>
>
> Those two files are different in our repo, and as such git thinks that
> we modified one of them when we try and pull it down from github
> again.
>
>
> Thanks for looking at this!
> -- Steven
I assume that you are on Mac OS ?
This is what I would have done:
- find the twin of your file:
$ git ls-files | grep -i tests/test_system/show_19_L.txt
- Let's assume it is the little brother:
"tests/test_system/show_19_l.txt"
$ git mv tests/test_system/show_19_l.txt tests/test_system/show_19_l2.txt
- Check out the original:
$ git checkout tests/test_system/show_19_L.txt
- check:
$ git status
^ permalink raw reply
* "git fsck" not detecting garbage at the end of blob object files...
From: John Szakmeister @ 2017-01-07 12:50 UTC (permalink / raw)
To: git
I was perusing StackOverflow this morning and ran across this
question: http://stackoverflow.com/questions/41521143/git-fsck-full-only-checking-directories/
It was a simple question about why "checking objects" was not
appearing, but in it was another issue. The user purposefully
corrupted a blob object file to see if `git fsck` would catch it by
tacking extra data on at the end. `git fsck` happily said everything
was okay, but when I played with things locally I found out that `git
gc` does not like that extra garbage. I'm not sure what the trade-off
needs to be here, but my expectation is that if `git fsck` says
everything is okay, then all operations using that object (file)
should work too.
Is that unreasonable? What would be the impact of fixing this issue?
-John
^ permalink raw reply
* Re: [PATCH] rebase--interactive: count squash commits above 10 correctly
From: Jeff King @ 2017-01-07 11:05 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Brandon Tolsch, Vasco Almeida, git
In-Reply-To: <alpine.DEB.2.20.1701071150490.3469@virtualbox>
On Sat, Jan 07, 2017 at 11:51:23AM +0100, Johannes Schindelin wrote:
> > We can fix this by making the ".*" less greedy. Instead of
> > depending on ".*?" working portably, we can just limit the
> > match to non-digit characters, which accomplishes the same
> > thing.
>
> Or we could simply require a space before the first digit, which would
> maybe look a little simpler.
Yeah, if we can assume that all translations will always have a space
there. It is almost certainly true for Western languages, but I don't
know about others. It looks like the Korean translation omits a space
_after_ the digits, but there is still one before.
-Peff
^ permalink raw reply
* Re: [PATCH] rebase--interactive: count squash commits above 10 correctly
From: Johannes Schindelin @ 2017-01-07 10:51 UTC (permalink / raw)
To: Jeff King; +Cc: Brandon Tolsch, Vasco Almeida, git
In-Reply-To: <20170107082318.jj3izthx2ylscrns@sigill.intra.peff.net>
Hi Peff,
On Sat, 7 Jan 2017, Jeff King wrote:
> On Fri, Jan 06, 2017 at 01:04:05AM -0800, Brandon Tolsch wrote:
>
> We can fix this by making the ".*" less greedy. Instead of
> depending on ".*?" working portably, we can just limit the
> match to non-digit characters, which accomplishes the same
> thing.
Or we could simply require a space before the first digit, which would
maybe look a little simpler.
Ciao,
Dscho
^ permalink raw reply
* Re: [RFC PATCH 0/5] Localise error headers
From: Duy Nguyen @ 2017-01-07 9:34 UTC (permalink / raw)
To: Stefan Beller; +Cc: Michael J Gruber, git@vger.kernel.org
In-Reply-To: <CACsJy8B0UT4_CF=qu081ep6nzdBXxnnNbma-wCYeajAuXaKg5w@mail.gmail.com>
On Wed, Jan 4, 2017 at 8:25 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Wed, Jan 4, 2017 at 2:45 AM, Stefan Beller <sbeller@google.com> wrote:
>>> In this implementation, the gettext call for the header and the body are done
>>> in different places (error function vs. caller) but this call pattern seems to
>>> be the easiest variant for the caller, because the message body has to be marked
>>> for localisation in any case, and N_() requires more letters than _(), an extra
>>> argument to die() etc. even more than the extra "_" in the function name.
>>
>> I see. We have to markup the strings to be translatable such that the .po files
>> are complete. It would be really handy if there was a way to say "anything that
>> is fed to this function (die_) needs to be marked for translation.
>>
>> Looking through
>> https://www.gnu.org/software/gettext/manual/html_node/xgettext-Invocation.html
>> such a thing doesn't seem to exist.
>
> I think --keyword is exactly for that purpose: marking more text for
> translations besides standard markers like _() or N_(). Yes we need to
> call gettext() explicitly in die_() later on. We already do that for
> parse-options. We just need to N_() the strings, without actually
> spelling it out.
>
>>
>> So in that case die_(_(...)) seems to be the easiest way forward.
>
> I still prefer changing the die_routine though since die() in many
> cases could be used in both plumbing and porcelain contexts. And we
> have tried to keep plumbing output (and behavior) as stable as
> possible. The approach has some similarity to unpack_trees() which
> shares the same porcelain/plumbing problem.
On the other hand, making die(), not die_(), translatable means the
translators will have to translate them _all_ even if only some will
end up being displayed. That's 2000+ strings according to git-grep.
And some of them, like die("BUG:..") should definitely not be
translated. So +1 to die_(), unless we decide all strings are safe to
translate.
--
Duy
^ permalink raw reply
* Re: [PATCH v5 00/16] pathspec cleanup
From: Duy Nguyen @ 2017-01-07 9:29 UTC (permalink / raw)
To: Brandon Williams; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <20170104180411.150000-1-bmwill@google.com>
On Thu, Jan 5, 2017 at 1:03 AM, Brandon Williams <bmwill@google.com> wrote:
> Changes in v5:
> * Move GUARD_PATHSPEC to prevent checking if pathspec is null twice.
> * Mark a string containing 'mnemonic' for translation.
Argh.. I've run out of things to complain about! Ack!
--
Duy
^ permalink raw reply
* Re: [PATCH 3/3] execv_dashed_external: wait for child on signal death
From: Duy Nguyen @ 2017-01-07 9:07 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Sixt, Trygve Aaberge, Git Mailing List
In-Reply-To: <20170107012223.c27toqr6ck44kfpj@sigill.intra.peff.net>
On Sat, Jan 7, 2017 at 8:22 AM, Jeff King <peff@peff.net> wrote:
> When you hit ^C to interrupt a git command going to a pager,
> this usually leaves the pager running. But when a dashed
> external is in use, the pager ends up in a funny state and
> quits (but only after eating one more character from the
> terminal!). This fixes it.
>
> Explaining the reason will require a little background.
>
> ...
I see I have exposed a bug and I'm glad you are around to fix it. Even
reading your explanation is scary enough, let alone finding it.
Thanks!
--
Duy
^ permalink raw reply
* [BUG] push sometimes omits status table when remote dies
From: Jeff King @ 2017-01-07 8:48 UTC (permalink / raw)
To: git
If I run t5504 under heavy load, it sometimes fails tests 8 and 9, which
make a broken push to the remote. The failure looks like this:
expecting success:
rm -rf dst &&
git init dst &&
(
cd dst &&
git config transfer.fsckobjects true
) &&
test_must_fail git push --porcelain dst master:refs/heads/test >act &&
test_cmp exp act
Initialized empty Git repository in /var/ram/git-stress/root-13/trash directory.t5504-fetch-receive-strict/dst/.git/
remote: fatal: object of unexpected type
error: failed to push some refs to 'dst'
--- exp 2017-01-07 08:44:24.465771756 +0000
+++ act 2017-01-07 08:44:24.493771755 +0000
@@ -1,2 +0,0 @@
-To dst
-! refs/heads/master:refs/heads/test [remote rejected] (unpacker error)
not ok 9 - push with transfer.fsckobjects
So it looks like we correctly fail the push, but the expected porcelain
output is missing. I'm _guessing_ what happens is that the local
git-push sees SIGPIPE writing to the remote side, and dies before it
gets a chance to write the message.
I'm not sure if this is something we should be fixing, or if the test is
simply a bit flaky (and we should work around it).
-Peff
^ permalink raw reply
* [PATCH] rebase--interactive: count squash commits above 10 correctly
From: Jeff King @ 2017-01-07 8:23 UTC (permalink / raw)
To: Brandon Tolsch; +Cc: Johannes Schindelin, Vasco Almeida, git
In-Reply-To: <CAMWRQeRaQQQcJ-R8eHc7f0KqZF2eEkYJOyTb9n7ds78pTqV-AA@mail.gmail.com>
On Fri, Jan 06, 2017 at 01:04:05AM -0800, Brandon Tolsch wrote:
> git --version: 2.11.0
>
> When using git rebase -i to squash a series of commits that includes
> more than 10 commits, the generated commit message you are given to
> edit counts the old messages incorrectly. It will say the total
> number of commits is (actual % 10) (if they were 0-based) and it will
> also count the commits as (actual % 10).
Looks like a regression in v2.10. Here's the fix.
-- >8 --
Subject: rebase--interactive: count squash commits above 10 correctly
We generate the squash commit message incrementally running
a sed script once for each commit. It parses "This is
a combination of <N> commits" from the first line of the
existing message, adds one to <N>, and uses the result as
the number of our current message.
Since f2d17068fd (i18n: rebase-interactive: mark comments of
squash for translation, 2016-06-17), the first line may be
localized, and sed uses a pretty liberal regex, looking for:
/^#.*([0-9][0-9]*)/
The "[0-9][0-9]*" tries to match double digits, but it
doesn't quite work. The first ".*" is greedy, so if you
have:
This is a combination of 10 commits.
it will eat up "This is a combination of 1", leaving "0" to
match the first "[0-9]" digit, and then skipping the
optional match of "[0-9]*".
As a result, the count resets every 10 commits, and a
15-commit squash would end up as:
# This is a combination of 5 commits.
# This is the 1st commit message:
...
# This is the commit message #2:
... and so on ..
# This is the commit message #10:
...
# This is the commit message #1:
...
# This is the commit message #2:
... etc, up to 5 ...
We can fix this by making the ".*" less greedy. Instead of
depending on ".*?" working portably, we can just limit the
match to non-digit characters, which accomplishes the same
thing.
Reported-by: Brandon Tolsch <btolsch@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
I didn't include a test here, mostly because this bug is so weirdly
specific that it seems unlikely to happen again. Especially in light of
this code going away in favor of the C rebase helper, which Dscho
already confirmed is not buggy (and I did not look at the code, but it
cannot possibly do anything as gross as these repeated sed invocations).
It also is a little tricky to automatically extract the comments (you
have to override GIT_EDITOR, but we also invoke GIT_EDITOR for the insn
sheet).
I was able to replicate and confirm the fix manually with:
git commit -q --allow-empty -m base
git commit -q --allow-empty -m squash
git tag base
for i in $(seq 15); do
echo $i >$i
git add $i
git commit -qm "squash! squash"
done
git rebase --autosquash -i base
I'd also suggest that "the commit message #4" is grammatically
questionable. Probably "This is commit message #4" would be fine.
git-rebase--interactive.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index b0a6f2b7ba..4734094a3f 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -425,7 +425,7 @@ update_squash_messages () {
if test -f "$squash_msg"; then
mv "$squash_msg" "$squash_msg".bak || exit
count=$(($(sed -n \
- -e "1s/^$comment_char.*\([0-9][0-9]*\).*/\1/p" \
+ -e "1s/^$comment_char[^0-9]*\([0-9][0-9]*\).*/\1/p" \
-e "q" < "$squash_msg".bak)+1))
{
printf '%s\n' "$comment_char $(eval_ngettext \
--
2.11.0.527.gfef230ca76
^ permalink raw reply related
* Re: [PATCH 3/3] execv_dashed_external: wait for child on signal death
From: Jeff King @ 2017-01-07 7:34 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Trygve Aaberge, Nguyễn Thái Ngọc Duy, git
In-Reply-To: <650a25f6-5f22-8efb-3048-6afadbaa7092@kdbg.org>
On Sat, Jan 07, 2017 at 08:28:22AM +0100, Johannes Sixt wrote:
> > But when you have a dashed external (or an alias pointing to
> > a builtin, which will re-exec git for the builtin), there's
> > an extra process in the mix. For instance, running:
> >
> > git -c alias.l=log log
>
> This should be
>
> git -c alias.l=log l
Yeah, it should be.
> For the complete series:
>
> Acked-by: Johannes Sixt <j6t@kdbg.org>
Thanks.
> What should we add to Documentation/technical/api-run-command.txt about the
> new flag? "Do not use?" "Understand the commit message of <this commit>
> before setting the flag to true?"
I think it can be explained pretty easily as "after killing all children
marked for clean_on_exit, do a blocking waitpid() on any child that is
also marked wait_after_clean". But I notice we do not actually document
clean_on_exit, either, nor most of the options in "struct
child_process".
IMHO it would be an improvement to merge the contents of the
technical/api-run-command.txt documentation into the header file, and
document each option with a comment above its definition. That makes it
a lot easier to have the will to document any newly-added options,
because if you don't they look bad next to the others. :)
Mechanics aside, I am not sure if we need a "do not use" or "here are
the caveats". I think if we explain what it does, we can rely on list
discussion and review to catch any obviously-stupid uses (both of it and
of clean_on_exit).
-Peff
^ permalink raw reply
* Re: [PATCH 3/3] execv_dashed_external: wait for child on signal death
From: Johannes Sixt @ 2017-01-07 7:28 UTC (permalink / raw)
To: Jeff King; +Cc: Trygve Aaberge, Nguyễn Thái Ngọc Duy, git
In-Reply-To: <20170107012223.c27toqr6ck44kfpj@sigill.intra.peff.net>
Am 07.01.2017 um 02:22 schrieb Jeff King:
> When you hit ^C to interrupt a git command going to a pager,
> this usually leaves the pager running. But when a dashed
> external is in use, the pager ends up in a funny state and
> quits (but only after eating one more character from the
> terminal!). This fixes it.
>
> Explaining the reason will require a little background.
>
> When git runs a pager, it's important for the git process to
> hang around and wait for the pager to finish, even though it
> has no more data to feed it. This is because git spawns the
> pager as a child, and thus the git process is the session
> leader on the terminal. After it dies, the pager will finish
> its current read from the terminal (eating the one
> character), and then get EIO trying to read again.
>
> When you hit ^C, that sends SIGINT to git and to the pager,
> and it's a similar situation. The pager ignores it, but the
> git process needs to hang around until the pager is done. We
> addressed that long ago in a3da882120 (pager: do
> wait_for_pager on signal death, 2009-01-22).
>
> But when you have a dashed external (or an alias pointing to
> a builtin, which will re-exec git for the builtin), there's
> an extra process in the mix. For instance, running:
>
> git -c alias.l=log log
This should be
git -c alias.l=log l
>
> will end up with a process tree like:
>
> git (parent)
> \
> git-log (child)
> \
> less (pager)
>
> If you hit ^C, SIGINT goes to all of them. The pager ignores
> it, and the child git process will end up in wait_for_pager().
> But the parent git process will die, and the usual EIO
> trouble happens.
>
> So we really want the parent git process to wait_for_pager(),
> but of course it doesn't know anything about the pager at
> all, since it was started by the child. However, we can
> have it wait on the git-log child, which in turn is waiting
> on the pager. And that's what this patch does.
>
> There are a few design decisions here worth explaining:
>
> 1. The new feature is attached to run-command's
> clean_on_exit feature. Partly this is convenience,
> since that feature already has a signal handler that
> deals with child cleanup.
>
> But it's also a meaningful connection. The main reason
> that dashed externals use clean_on_exit is to bind the
> two processes together. If somebody kills the parent
> with a signal, we propagate that to the child (in this
> instance with SIGINT, we do propagate but it doesn't
> matter because the original signal went to the whole
> process group). Likewise, we do not want the parent
> to go away until the child has done so.
>
> In a traditional Unix world, we'd probably accomplish
> this binding by just having the parent execve() the
> child directly. But since that doesn't work on Windows,
> everything goes through run_command's more spawn-like
> interface.
>
> 2. We do _not_ automatically waitpid() on any
> clean_on_exit children. For dashed externals this makes
> sense; we know that the parent is doing nothing but
> waiting for the child to exit anyway. But with other
> children, it's possible that the child, after getting
> the signal, could be waiting on the parent to do
> something (like closing a descriptor). If we were to
> wait on such a child, we'd end up in a deadlock. So
> this errs on the side of caution, and lets callers
> enable the feature explicitly.
>
> 3. When we send children the cleanup signal, we send all
> the signals first, before waiting on any children. This
> is to avoid the case where one child might be waiting
> on another one to exit, causing a deadlock. We inform
> all of them that it's time to die before reaping any.
>
> In practice, there is only ever one dashed external run
> from a given process, so this doesn't matter much now.
> But it future-proofs us if other callers start using
> the wait_after_clean mechanism.
>
> There's no automated test here, because it would end up racy
> and unportable. But it's easy to reproduce the situation by
> running the log command given above and hitting ^C.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> git.c | 1 +
> run-command.c | 19 +++++++++++++++++++
> run-command.h | 1 +
> 3 files changed, 21 insertions(+)
>
> diff --git a/git.c b/git.c
> index bc2f2a7ec9..c8fe6637df 100644
> --- a/git.c
> +++ b/git.c
> @@ -588,6 +588,7 @@ static void execv_dashed_external(const char **argv)
> argv_array_pushf(&cmd.args, "git-%s", argv[0]);
> argv_array_pushv(&cmd.args, argv + 1);
> cmd.clean_on_exit = 1;
> + cmd.wait_after_clean = 1;
> cmd.silent_exec_failure = 1;
>
> trace_argv_printf(cmd.args.argv, "trace: exec:");
> diff --git a/run-command.c b/run-command.c
> index ca905a9e80..73bfba7ef9 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -29,6 +29,8 @@ static int installed_child_cleanup_handler;
>
> static void cleanup_children(int sig, int in_signal)
> {
> + struct child_to_clean *children_to_wait_for = NULL;
> +
> while (children_to_clean) {
> struct child_to_clean *p = children_to_clean;
> children_to_clean = p->next;
> @@ -45,6 +47,23 @@ static void cleanup_children(int sig, int in_signal)
> }
>
> kill(p->pid, sig);
> +
> + if (p->process->wait_after_clean) {
> + p->next = children_to_wait_for;
> + children_to_wait_for = p;
> + } else {
> + if (!in_signal)
> + free(p);
> + }
> + }
> +
> + while (children_to_wait_for) {
> + struct child_to_clean *p = children_to_wait_for;
> + children_to_wait_for = p->next;
> +
> + while (waitpid(p->pid, NULL, 0) < 0 && errno == EINTR)
> + ; /* spin waiting for process exit or error */
> +
> if (!in_signal)
> free(p);
> }
> diff --git a/run-command.h b/run-command.h
> index dd1c78c28d..4fa8f65adb 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -43,6 +43,7 @@ struct child_process {
> unsigned stdout_to_stderr:1;
> unsigned use_shell:1;
> unsigned clean_on_exit:1;
> + unsigned wait_after_clean:1;
> void (*clean_on_exit_handler)(struct child_process *process);
> void *clean_on_exit_handler_cbdata;
> };
>
Very nice write-up and an "obviously correct patch" (FLW).
For the complete series:
Acked-by: Johannes Sixt <j6t@kdbg.org>
What should we add to Documentation/technical/api-run-command.txt about
the new flag? "Do not use?" "Understand the commit message of <this
commit> before setting the flag to true?"
-- Hannes
^ permalink raw reply
* Re: [ANNOUNCE] git-test: run automated tests against a range of Git commits
From: Jeff King @ 2017-01-07 7:18 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git discussion list
In-Reply-To: <1341c01a-aca7-699c-c53a-28d048614bfe@alum.mit.edu>
On Fri, Jan 06, 2017 at 04:52:16PM +0100, Michael Haggerty wrote:
> I just released `git test`, a script for running automated
> tests across a range of Git commits and keeping track of the results in
> git notes:
>
> https://github.com/mhagger/git-test
>
> This is a script that I've been using in one form or another for years
> and I find it really handy [1].
Neat. I usually "git rebase -x 'make -j8 test' @{u}" after finishing a
topic to make sure the intermediate steps are good. But it would be neat
to have this running continuously in the background to alert me to
problems sooner (and the key thing there is that it remembers
already-run tests, so it should be safe to basically for new commits
every 10 seconds or so).
I did hit a few interesting cases trying out "git test". So here's a
narrative, and you can pick out where there may be room for improvement
in the tool, and where I'm just being dumb. :)
I tried it out first on a topic I finished earlier today, which has 3
commits. So I did:
$ git test add 'make -j8 test'
$ git test range @{u}..HEAD
It barfed on the first commit, because the script expects "git co" to
work, but I don't have that alias. No big deal (and I already submitted
a PR to fix it).
So then I reinvoked it like:
$ git test range @{u}..HEAD
and it actually ran some tests. Yay.
And then of course I wanted to prove to myself how cool the notes
feature is, so I ran it again. It didn't run any tests this time. Yay
again. But there were a few surprises:
$ git test range @{u}..HEAD
setup_test default
Using test default; command: make -j8 test
Old status: bad
Tree 9fcdbd5c78^{tree} is already known to be bad!
Old status: good
Tree c22f4f6624^{tree} is already known to be good.
Old status: good
Tree 19e2e62e5e^{tree} is already known to be good.
Already on 'jk/wait-for-child-cleanup'
Your branch is ahead of 'origin/master' by 3 commits.
ALL TESTS SUCCESSFUL
My initial run with "git co" had left the first commit marked as "bad".
That's not _too_ surprising, since it did indeed fail. I think it's
probably a bug to record a failure note, though, if checking out fails.
It's not necessarily an immutable property of the tree. In my case, it
was obviously dependent on a change in the git-test code, but that's
hopefully a fairly uncommon occurrence. But there are other reasons for
git-checkout to complain. For instance, imagine topic "foo" creates file
"foo.c". If I do:
$ echo content >foo.c
$ git test foo@{u}..foo
then checkout will complain about overwriting the untracked "foo.c".
It's reasonable to abort the operation there, but probably not to write
a permanent failure note. The problem wasn't in "foo", but in my local
tree.
The second thing that surprised me was "ALL TESTS SUCCESSFUL", when
clearly one of them was known-bad. :)
So at this point I knew I needed to re-run the test. Looks like there's
a "--force" option. Let's try it. There's no need to re-run the other
two, so let's just give it one commit:
$ git test range -f HEAD~2
...
Object 95649d6cf9ec68f05d1dc57ec1b989b8d263a7ae^{tree} has no note
Object e1970ce43abfbf625bce68516857e910748e5965^{tree} has no note
Object 368f99d57e8ed17243f2e164431449d48bfca2fb^{tree} has no note
Object ceede59ea90cebad52ba9c8263fef3fb6ef17593^{tree} has no note
Object dfe070511c652f2b8e1bf6540f238c9ca9ba41d3^{tree} has no note
Object 902d960b382a0cd424618ff4e1316da40e4be2f6^{tree} has no note
...
This started spewing out many lines like the one above, until I hit ^C.
Yikes!
Thinking something was wrong with the "-f" option, I tried it without:
$ git test range -f HEAD~2
...
commit e83c5163316f89bfbde7d9ab23ca2e25604af290 (origin/initial)
Author: Linus Torvalds <torvalds@ppc970.osdl.org>
Date: Thu Apr 7 15:13:13 2005 -0700
Initial revision of "git", the information manager from hell
Oops. Now I see the problem. I was expecting the arguments to be
rebase-like. But they're really rev-list like. So in both cases it
wanted to test all the way up from the root commits to HEAD~2. The "-f"
version never got to testing a commit because it was so busy trying to
delete the old notes.
I see the symmetry and simplicity in allowing the user to specify a full
range. But it also seems like it's easy to make a mistake that's going
to want to test a lot of commits. I wonder if it should complain when
there's no lower bound to the commit range. Or alternatively, if there's
a single positive reference, treat it as a lower bound, with HEAD as the
upper bound (which is vaguely rebase-like).
A few other observations about the note deletion:
- The "has no note" message should perhaps be suppressed. We're just
trying to overwrite the value if there is one (alternatively,
instead of removing it, just overwrite it, so the old note stays
until we get a result one way or the other).
- It was sufficiently slow that it looks like we invoke "git notes
remove" once per commit. It would be a lot more efficient to batch
them (not just in terms of process startup, but because you're going
to write a _ton_ of intermediate notes trees).
Of course none of that matters if you don't do something stupid like
trying to "git test" 45,000 commits. :)
So OK. Now I know what's going on. And I can get what I want with:
$ git test range -f HEAD~3..HEAD~2
which works, and now all of my tests are correctly marked. Of course
that's a lot to type. It would be easier as:
$ git test range -f -1 HEAD~2
usage: git-test [-h] {add,range,help} ...
git-test: error: unrecognized arguments: HEAD~2
but the tool doesn't seem to like passing through the rev-list argument.
It would be even easier if I could just repeat my range and only re-test
the "bad" commits. It was then that I decided to actually read the rest
of "git test help range" and see that you already wrote such an option,
cleverly hidden under the name "--retest".
And one final nit. I notice there is also a "--keep-going" option. Which
made me surprised that we bothered to test HEAD~1 and HEAD, when we knew
that HEAD~2 was bogus. I suspect this is related to the "ALL TESTS
SUCCESSFUL" issue. Presumably cached test results are not treated as
"bad" in general, which seems funny to me.
So those were all little cosmetic things. The other big thing I wanted
to see was what it's like to fix a bug deep in a topic. So I used "git
rebase -i" to inset a compile error into the first commit of my 3-patch
series. And then I tested it:
$ git test add -t compile 'make -j8'
$ git test range -t compile HEAD~3..
As predicted, it stopped at the first commit and told me it was buggy.
But I'm dumped onto a detached HEAD, and I'm on my own to actually get
the working tree to a state where I can test and fix on my actual
branch.
That's a nice outcome of the "git rebase -x make" approach. When you hit
a bug, you can fix it, "git commit --amend", and "git rebase
--continue". The downside is that it takes ownership of the branch while
you're doing it. So the whole concept of "run this in the background
with a separate worktree" breaks down completely.
I think it should be possible to script the next steps, though.
Something like like "git test fix foo", which would:
- expand the range of foo@{u}..foo to get the list of commits
- see which ones were marked as broken
- kick off an interactive rebase, but override GIT_EDITOR to mark any
broken ones as "edit" instead of "pick"
That lets you separate the act of testing from the act of fixing. You
can let the tester run continuously in the background, and only stop to
fix when you're at an appropriate point in your work.
Anyway. Seems like a neat tool. I may play around with it and see if I
can fit it into my workflow.
-Peff
^ permalink raw reply
* Re: [PATCH v2 4/4] mergetool: fix running in subdir when rerere enabled
From: Richard Hansen @ 2017-01-07 2:16 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git, davvid, sbeller, simon
In-Reply-To: <199807ae-844c-57cd-28cf-2c10b3aee7a9@kdbg.org>
[-- Attachment #1: Type: text/plain, Size: 2562 bytes --]
On 2017-01-06 04:42, Johannes Sixt wrote:
> Am 06.01.2017 um 02:09 schrieb Richard Hansen:
>> If rerere is enabled and no pathnames are given, run cd_to_toplevel
>> before running 'git diff --name-only' so that 'git diff --name-only'
>> sees the pathnames emitted by 'git rerere remaining'.
>>
>> Also run cd_to_toplevel before running 'git rerere remaining' in case
>> 'git rerere remaining' is ever changed to print pathnames relative to
>> the current directory rather than to $GIT_WORK_TREE.
>>
>> This fixes a regression introduced in
>> 57937f70a09c12ef484c290865dac4066d207c9c (v2.11.0).
>>
>> Signed-off-by: Richard Hansen <hansenr@google.com>
>> ---
>> git-mergetool.sh | 1 +
>> t/t7610-mergetool.sh | 2 +-
>> 2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/git-mergetool.sh b/git-mergetool.sh
>> index e52b4e4f2..67ea0d6db 100755
>> --- a/git-mergetool.sh
>> +++ b/git-mergetool.sh
>> @@ -456,6 +456,7 @@ main () {
>>
>> if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR"
>> then
>> + cd_to_toplevel
>> set -- $(git rerere remaining)
>> if test $# -eq 0
>> then
>
> This cannot be a complete solution. Why do we have another
> cd_to_toplevel later, after `git diff --name-only -- "$@"`?
The arguments passed to 'git diff' (including the -O argument) are all
interpreted as relative to the current working directory, yet 'git diff
--name-only' outputs pathnames that are relative to the top-level
directory. Thus:
* cd_to_toplevel MUST NOT be run before that 'git diff' command
unless all pathnames relative to $PWD are converted to absolute (or
relative to the top-level directory), and
* cd_to_toplevel MUST be run after 'git diff' so that $files is
interpreted correctly.
And now I realize that my change breaks -O<foo> if <foo> is relative to
$PWD. Grr. Too bad we don't have tests for running mergetool
-O<relative-path> from a subdirectory.
>
> Maybe it is necessary to revert back to the flow control that we had
> before 57937f70a09c ("mergetool: honor diff.orderFile", 2016-10-07)? It
> did not have `test $# -eq 0` and `test -e "$GIT_DIR/MERGE_RR"` in a
> single condition.
Reverting to the previous control flow won't work unless the -O pathname
is converted to absolute (or relative to the top-level directory). But
I'll have to do that anyway. Blech.
Do we already have a helper shell function somewhere that converts a
pathname to absolute? Thanks to symlinks it's trickier than one might
expect.
Thanks,
Richard
>
> -- Hannes
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]
^ permalink raw reply
* Re: [PATCH v2 2/4] t7610: make tests more independent and debuggable
From: Richard Hansen @ 2017-01-07 1:53 UTC (permalink / raw)
To: Stefan Beller
Cc: git@vger.kernel.org, David Aguilar, Johannes Sixt, Simon Ruderich
In-Reply-To: <CAGZ79kbRee+3MbAHCSFB0QqGMMF5bcZMiEHV-coRh87vFfq0Ag@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3039 bytes --]
On 2017-01-05 20:31, Stefan Beller wrote:
> On Thu, Jan 5, 2017 at 5:09 PM, Richard Hansen <hansenr@google.com> wrote:
>> Reduce how much a test can interfere other tests:
>
> A bullet point list as an unordered list often indicates that you're
> doing multiple
> things at once, possibly unrelated, so they could go into different patches. ;)
OK, I'll split it up.
>
> While telling you to make even more commits: you may also want to make
> a patch with an entry to the .mailmap file (assuming you're the same
> Richard Hansen that contributed from rhansen@bbn.com);
> Welcome to Google!
Good idea, thanks!
>
>>
>> * Move setup code that multiple tests depend on to the 'setup' test
>> case.
>> * Run 'git reset --hard' after every test (pass or fail) to clean up
>> in case the test fails and leaves the repository in a strange
>> state.
>> * If the repository must be in a particular state (beyond what is
>> already done by the 'setup' test case) before the test can run,
>> make the necessary repository changes in the test script even if
>> it means duplicating some lines of code from the previous test
>> case.
>> * Never assume that a particular commit is checked out.
>> * Always work on a test-specific branch when the test might create a
>> commit. This is not always necessary for correctness, but it
>> improves debuggability by ensuring a commit created by test #N
>> shows up on the testN branch, not the branch for test #N-1.
>
>
>
>
>> @@ -112,6 +146,7 @@ test_expect_success 'custom mergetool' '
>> '
>>
>> test_expect_success 'mergetool crlf' '
>> + test_when_finished "git reset --hard" &&
>> test_config core.autocrlf true &&
>> git checkout -b test$test_count branch1 &&
>> test_must_fail git merge master >/dev/null 2>&1 &&
>> @@ -129,11 +164,11 @@ test_expect_success 'mergetool crlf' '
>> git submodule update -N &&
>> test "$(cat submod/bar)" = "master submodule" &&
>> git commit -m "branch1 resolved with mergetool - autocrlf" &&
>
>> - test_config core.autocrlf false &&
>> - git reset --hard
>> + test_config core.autocrlf false
>> '
>
> This is the nit that led me to writing this email in the first place:
> test_config is a function that sets a configuration for a single test only,
> so it makes no sense as the last statement of a test. (In its implementation
> it un-configures with test_when_finished)
>
> So I think we do not want to add it back, but rather remove this
> test_config statement.
OK, will do.
>
> But to do this we need to actually be careful with the order of the newly
> added test_when_finished "git reset --hard" and test_config core.autocrlf true,
> which uses test_when_finished internally.
Ah, yes. Tricky. I'll add a comment.
>
> The order seems correct to me, as the reset would be executed after the
> "test_config core.autocrlf true" is un-configured.
Agreed; test_when_finished is LIFO (though the order is not documented).
-Richard
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]
^ permalink raw reply
* Re: [PATCH 4/5] unpack-trees: factor file removal out of check_updates
From: Jeff King @ 2017-01-07 1:36 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, l.s.r, git
In-Reply-To: <20170106210330.31761-5-sbeller@google.com>
On Fri, Jan 06, 2017 at 01:03:29PM -0800, Stefan Beller wrote:
> +static int remove_workingtree_files(struct unpack_trees_options *o,
> + struct progress *progress)
> +{
> + int i;
> + unsigned cnt = 0;
> + struct index_state *index = &o->result;
> +
> + for (i = 0; i < index->cache_nr; i++) {
> + const struct cache_entry *ce = index->cache[i];
> +
> + if (ce->ce_flags & CE_WT_REMOVE) {
> + display_progress(progress, ++cnt);
> + if (o->update && !o->dry_run)
> + unlink_entry(ce);
> + }
> + }
> +
> + return cnt;
> +}
"cnt" is unsigned here, as it is in the caller. Should the return value
match?
-Peff
^ permalink raw reply
* Re: [PATCH] branch_get_push: do not segfault when HEAD is detached
From: Jeff King @ 2017-01-07 1:31 UTC (permalink / raw)
To: Kyle Meyer; +Cc: Johannes Schindelin, git
In-Reply-To: <87bmvjll8m.fsf@kyleam.com>
On Fri, Jan 06, 2017 at 08:19:53PM -0500, Kyle Meyer wrote:
> > The other option is just "git checkout --detach", which is also used in
> > the test suite. I tend to prefer it because it's a little more obvious
> > to a reader.
>
> True, that does seem clearer. Seems I should've waited a bit before
> sending out v2.
I think it's OK either way. Junio can also mark it up while applying,
too, if he has a preference.
-Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox