* Re: [PATCH 07/34] sequencer (rebase -i): add support for the 'fixup' and 'squash' commands
From: Johannes Schindelin @ 2016-09-02 7:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Dennis Kaarsemaker, git
In-Reply-To: <xmqqr393wkof.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Thu, 1 Sep 2016, Junio C Hamano wrote:
> For those who were not paying attention on the 'master' front during
> this pre-release period [*1*], I have to point out that the scripted
> Porcelain has been updated to lose the Anglo-centric st/nd/rd/th and
> this series would want to get updated to match.
>
>
> [Footnote]
>
> *1* Why weren't you? Repent! ;-)
I tried to. But, you know, I was kinda busy with a couple of patch series.
In any case, I changed the code this morning. Can't say that I like those
forced last-minute changes.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH 07/34] sequencer (rebase -i): add support for the 'fixup' and 'squash' commands
From: Johannes Schindelin @ 2016-09-02 7:13 UTC (permalink / raw)
To: Dennis Kaarsemaker; +Cc: git, Junio C Hamano
In-Reply-To: <1472746749.4680.33.camel@kaarsemaker.net>
Hi Dennis,
On Thu, 1 Sep 2016, Dennis Kaarsemaker wrote:
> On do, 2016-09-01 at 17:17 +0200, Johannes Schindelin wrote:
>
> > And I see that the beautiful ordinal computation was given up in favor
> > of a lousy "#1", "#2", "#3", etc (it used to be "1st", "2nd", "3rd"
> > etc).
> >
> > In any case, translation is not my main concern until v2.10.0, so I'll
> > take care of this after that release.
>
> Hmm, not sure if I agree with that. I'd see it as a regression to lose
> the i18n there.
As Git for Windows does not ship with translations (for multiple reasons),
it would not be a regression.
Having said that, I see that having a different text than the current
rebase -i can be seen as a regression, so I changed that.
Thanks for the review!
Dscho
^ permalink raw reply
* Re: bitmap creation failed
From: Jeff King @ 2016-09-02 6:41 UTC (permalink / raw)
To: Arumuga; +Cc: git
In-Reply-To: <CAJ23aQE2YsizuKywGmKn83jspL0JZin93drPc=QzFLo+hp8EwA@mail.gmail.com>
On Fri, Sep 02, 2016 at 12:04:54PM +0530, Arumuga wrote:
> So I understand now, the following.
>
> 1. reducing the pack file size will increase the clone time
> 2. Single pack file is expected to better use bitmap feature.
>
> Am i correct ?
Yes, on both.
-Peff
^ permalink raw reply
* Re: bitmap creation failed
From: Jeff King @ 2016-09-02 1:09 UTC (permalink / raw)
To: gjarms; +Cc: git
In-Reply-To: <1472753809733-7657450.post@n2.nabble.com>
On Thu, Sep 01, 2016 at 11:16:49AM -0700, gjarms wrote:
> Hi Git Experts,
>
> We have been exploring various ways to improve git cloning time, one among
> them is using bitmap which is suppose to save time "counting objects". but
> i have problem creating bitmap since the repository contains 100's of pack
> files. the bitmap file is not created when i use "git gc".
>
> I have the following entries in my .gitconfig.
>
> [pack]
> packSizeLimit = 10m
> writebitmaps = on
> writeBitmapHashCache = on
>
> If i just dont use "packSizeLimit = 10m", then bitmap is created just by
> running git gc
>
> Can you please make me understand ?, What i understood is that the bitmap is
> created when there is a single pack file, but if i split it into multiple
> pack file, the bitmap generation fails with the warning
> "warning: disabling bitmap writing, as some objects are not being packed".
That's weird. I'd expect it to say:
disabling bitmap writing, packs are split due to pack.packSizeLimit
At least since v2.8.3, which has 9cea46cdda. Before that it wouldn't
have printed anything, and just silently turned off bitmaps.
The "some objects are not being packed" warning should only come when
want_object_in_pack() says we don't want an object. That's generally
because the object is either found in a shared alternates repository, or
is in a .keep pack. Though in the latter case, unless you've set
repack.packKeptObjects manually, we'll pack it anyway when bitmaps are
in effect (since ee34a2bead, in v2.0.0).
That confusion aside, you almost certainly should not be setting
packSizeLimit, and definitely not to something so low. Git will not
store cross-pack deltas, so you miss out on tons of delta opportunities.
As a result:
1. Your on-disk repository size will balloon. So you'll have a hundred
10m packs rather than one 200mb pack.
2. Your clone times will also grow, as git will try to find new deltas
between the objects in various packs independently for each clone.
-Peff
^ permalink raw reply
* Re: Should "git symbolic-ref -d HEAD" be forbidden?
From: Jeff King @ 2016-09-02 0:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq8tvbuu9r.fsf@gitster.mtv.corp.google.com>
On Thu, Sep 01, 2016 at 03:31:28PM -0700, Junio C Hamano wrote:
> -- >8 --
> Subject: symbolic-ref -d: do not allow removal of HEAD
>
> If you delete the symbolic-ref HEAD from a repository, Git no longer
> considers it valid, and even "git symbolic-ref HEAD refs/heads/master"
> would not be able to recover from that state.
>
> In the spirit similar to afe5d3d5 ("symbolic ref: refuse non-ref
> targets in HEAD", 2009-01-29), forbid removal of HEAD.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
Makes sense. You might want to change "it" in "no longer considers it
valid" to "the repository". At first I thought "it" referred to the
symref. Which obviously shouldn't be valid after being deleted. :)
> I decided against it for now for no good reason, other than I am a
> bit superstitious, but it may be a good idea to move these safety
> checks to delete_ref() and create_symref() in the longer term.
Yeah, that somehow feels weird and too low-level to me. After all, we
_do_ want to drop HEAD as a symref when we turn it into a detached HEAD.
The point of this (and afe5d3d5) is to prevent people from shooting
themselves in the foot. Internal Git code should know to avoid this
foot-shooting itself.
OTOH, I think "git update-ref --no-deref -d HEAD" is another user-facing
hole-in-foot opportunity, and it would be blocked by putting this into
delete_ref().
> -test_expect_success 'symbolic-ref deletes HEAD' '
> - git symbolic-ref -d HEAD &&
> +test_expect_success 'HEAD cannot be removed' '
> + test_must_fail git symbolic-ref -d HEAD
> +'
> +
> +test_expect_success 'symbolic-ref can be deleted' '
> + git symbolic-ref NOTHEAD refs/heads/foo &&
> + git symbolic-ref -d NOTHEAD &&
> test_path_is_file .git/refs/heads/foo &&
> - test_path_is_missing .git/HEAD
> + test_path_is_missing .git/NOTHEAD
> '
> reset_to_sane
Do you want another "reset_to_sane" call after your new test? Otherwise
if it fails the "symbolic-ref can be deleted" test will start operating
on the parent repository.
-Peff
^ permalink raw reply
* Re: bitmap creation failed
From: Stefan Beller @ 2016-09-01 23:37 UTC (permalink / raw)
To: gjarms; +Cc: git@vger.kernel.org
In-Reply-To: <1472753809733-7657450.post@n2.nabble.com>
On Thu, Sep 1, 2016 at 11:16 AM, gjarms <gjarms@gmail.com> wrote:
> Hi Git Experts,
>
> We have been exploring various ways to improve git cloning time, one among
> them is using bitmap which is suppose to save time "counting objects". but
> i have problem creating bitmap since the repository contains 100's of pack
> files. the bitmap file is not created when i use "git gc".
>
> I have the following entries in my .gitconfig.
>
> [pack]
> packSizeLimit = 10m
> writebitmaps = on
> writeBitmapHashCache = on
>
> If i just dont use "packSizeLimit = 10m", then bitmap is created just by
> running git gc
>
> Can you please make me understand ?, What i understood is that the bitmap is
> created when there is a single pack file, but if i split it into multiple
> pack file, the bitmap generation fails with the warning
> "warning: disabling bitmap writing, as some objects are not being packed".
So I guess your single pack is larger than 10m, so it tries to create
multiple packs,
and that is not supported as bitmaps only operate on one pack.
Stefan
>
> Regards,
> Arumuga
>
>
>
> --
> View this message in context: http://git.661346.n2.nabble.com/bitmap-creation-failed-tp7657450.html
> Sent from the git mailing list archive at Nabble.com.
^ permalink raw reply
* Re: [PATCH 22/22] sequencer: refactor write_message()
From: Jakub Narębski @ 2016-09-01 23:35 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1609011619220.129229@virtualbox>
Hello Johannes,
W dniu 01.09.2016 o 16:20, Johannes Schindelin pisze:
> On Thu, 1 Sep 2016, Jakub Narębski wrote:
>> W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:
>>> if (commit_lock_file(&msg_file) < 0)
>>> return error(_("Error wrapping up %s."), filename);
>>
>> Another "while at it"... though the one that can be safely postponed
>> (well, the make message easier to understand part, not the quote
>> filename part):
>>
>> return error(_("Error wrapping up writing to '%s'."), filename);
>
> As I inherited this message, I'll keep it.
Well, please then add quotes while at it, at least, for consistency
return error(_("Error wrapping up '%s'."), filename);
Best,
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH 21/22] sequencer: left-trim the lines read from the script
From: Jakub Narębski @ 2016-09-01 23:33 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1609011608440.129229@virtualbox>
Hello Johannes,
W dniu 01.09.2016 o 16:13, Johannes Schindelin pisze:
> On Thu, 1 Sep 2016, Jakub Narębski wrote:
>> 'bol' is beginning-of-line, isn't it (a complement to eol)?
>
> Yep. How did you guess? :-)
Wouldn't 'beg' and 'end' instead of 'bol' and 'eol' be easier
to understand, thus more readable?
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH 19/22] sequencer: support cleaning up commit messages
From: Jakub Narębski @ 2016-09-01 23:31 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1609011543290.129229@virtualbox>
Hello Johannes,
W dniu 01.09.2016 o 15:56, Johannes Schindelin pisze:
> On Thu, 1 Sep 2016, Jakub Narębski wrote:
>> It's a pity that emulation of named parameters in C requires
>> relying on designated inits from C99
>>
>> typedef struct {
>> double pressure, moles, temp;
>> } ideal_struct;
>>
>> #define ideal_pressure(...) ideal_pressure_base((ideal_struct){.pressure=1, \
>> .moles=1, .temp=273.15, __VA_ARGS__})
>>
>> double ideal_pressure_base(ideal_struct in)
>> {
>> return 8.314 * in.moles*in.temp/in.pressure;
>> }
>>
>> ... ideal_pressure(.moles=2, .temp=373.15) ...
Forgot to add citation:
[1] Ben Klemens "21st Century C: C Tips from the New School", 2nd Ed. (2014),
O'Reilly Media, chapter 10. "Better Structures", subsection
"Optional and Named Arguments"
>
> Yeah, that looks unwieldy ;-)
>
Declaration needs some trickery, but use is much, much more readable
(if we cannot use sensibly named variables for passing arguments):
ideal_pressure()
ideal_pressure(.temp=373.15)
ideal_pressure(.moles=2)
ideal_pressure(.moles=2, .temp=373.15)
It is even better if there are large amount of parameters:
res = amortization(.amount=200000, .inflation=3,
.show_table=0, .extra_payoff=100)
vs
double amortize(double amt, double rate, double inflation, int months,
int selloff_month, double extra_payoff, int verbose,
double *interest_pv, double *duration, double *monthly_payment);
But we can't use it in Git, anyway
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH 16/22] sequencer: prepare for rebase -i's GPG settings
From: Jakub Narębski @ 2016-09-01 23:21 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1609011516080.129229@virtualbox>
Hello Johannes,
W dniu 01.09.2016 o 15:33, Johannes Schindelin pisze:
> On Wed, 31 Aug 2016, Jakub Narębski wrote:
>> W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:
>>> @@ -471,17 +487,20 @@ int sequencer_commit(const char *defmsg, struct replay_opts *opts,
>>>
>>> if (IS_REBASE_I()) {
>>> env = read_author_script();
>>> - if (!env)
>>> + if (!env) {
>>> + const char *gpg_opt = gpg_sign_opt_quoted(opts);
>>> +
>>> return error("You have staged changes in your working "
>>> "tree. If these changes are meant to be\n"
>>> "squashed into the previous commit, run:\n\n"
>>> - " git commit --amend $gpg_sign_opt_quoted\n\n"
>>
>> How did this get expanded by error(), and why we want to replace
>> it if it works?
After writing this email, I got an idea on how it could work:
git-rebase script calls some C helper, which outputs above, and
output of this helper is eval'ed by script (with gpg_sign_opt_quoted
variable present in the environment)...
>
> It did not work. It was a place-holder waiting for this patch ;-)
>
... but it might have been simply copy'n'pasted from shell script
to C, literally.
>>
>>> + " git commit --amend %s\n\n"
>>> "If they are meant to go into a new commit, "
>>> "run:\n\n"
>>> - " git commit $gpg_sign_opt_quoted\n\n"
>>> + " git commit %s\n\n"
>>> "In both case, once you're done, continue "
>>> "with:\n\n"
>>> - " git rebase --continue\n");
>>> + " git rebase --continue\n", gpg_opt, gpg_opt);
>>
>> Instead of passing option twice, why not make use of %1$s (arg reordering),
>> that is
>>
>> + " git commit --amend %1$s\n\n"
>> [...]
>> + " git commit %1$s\n\n"
>
> Cute. But would this not drive the l10ners insane?
>
Shouldn't, as l10ners need to deal with arg reordering, because in different
languages the order of words might be different: %s %s in English may be
%2$s %1$s in other language, see example in
https://www.gnu.org/software/gettext/manual/gettext.html#c_002dformat-Flag
Best,
--
Jakub Narębski
^ permalink raw reply
* bitmap creation failed
From: gjarms @ 2016-09-01 18:16 UTC (permalink / raw)
To: git
Hi Git Experts,
We have been exploring various ways to improve git cloning time, one among
them is using bitmap which is suppose to save time "counting objects". but
i have problem creating bitmap since the repository contains 100's of pack
files. the bitmap file is not created when i use "git gc".
I have the following entries in my .gitconfig.
[pack]
packSizeLimit = 10m
writebitmaps = on
writeBitmapHashCache = on
If i just dont use "packSizeLimit = 10m", then bitmap is created just by
running git gc
Can you please make me understand ?, What i understood is that the bitmap is
created when there is a single pack file, but if i split it into multiple
pack file, the bitmap generation fails with the warning
"warning: disabling bitmap writing, as some objects are not being packed".
Regards,
Arumuga
--
View this message in context: http://git.661346.n2.nabble.com/bitmap-creation-failed-tp7657450.html
Sent from the git mailing list archive at Nabble.com.
^ permalink raw reply
* Re: [PATCH 13/22] sequencer: remember the onelines when parsing the todo file
From: Junio C Hamano @ 2016-09-01 22:59 UTC (permalink / raw)
To: Jakub Narębski; +Cc: Johannes Schindelin, git
In-Reply-To: <63004be3-04de-00c5-a94d-1b600201bc6f@gmail.com>
Jakub Narębski <jnareb@gmail.com> writes:
> I wonder how probable is situation where we save instruction sheet
> for interactive rebase, with shortened SHA-1, and during rebase
> shortened SHA-1 stops being unambiguous...
It is my understanding that the shortened ones are only for end-user
consumption. The insn sheet internally uses fully expanded form for
this exact reason, and then abbreviated back at each step before the
updated one is presented to the end-user. Uniqueness guarantee is
enforced with new objects created during each step taken into
account by doing it this way.
^ permalink raw reply
* Re: [PATCH 13/22] sequencer: remember the onelines when parsing the todo file
From: Jakub Narębski @ 2016-09-01 22:52 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git
In-Reply-To: <alpine.DEB.2.20.1609011507130.129229@virtualbox>
Hello,
W dniu 01.09.2016 o 15:12, Johannes Schindelin pisze:
> On Wed, 31 Aug 2016, Jakub Narębski wrote:
>> W dniu 31.08.2016 o 21:10, Junio C Hamano pisze:
>>> So I am not sure if we want a parsed commit there (I would not
>>> object if we kept the texual object name read from the file,
>>> though). The "one sequencer to rule them all" may even have to say
>>> "now give name ':1' to the result of the previous operation" in one
>>> step and in another later step have an instruction "merge ':1'".
>>> When that happens, you cannot even pre-populate the commit object
>>> when the sequencer reads the file, as the commit has not yet been
>>> created at that point.
>>
>> True, --preserve-merges rebase is well, different.
>
> It is mis-designed. And I can be that harsh because it was my design.
>
> In the meantime I came up with a much better design, and implemented it as
> a shell script on top of rebase -i. Since shell scripts run like slow
> molasses, even more so on Windows, I have a loose plan to implement its
> functionality as a new --recreate-merges option, and to deprecate
> --preserve-merges when that new option works.
>
> It needs to be a new option (not a --preserve-merges=v2) because it is a
> totally different beast. For starters, it does not need its own code path
> that overrides pick_one, as --preserve-merges does.
Better preserving for merges (with cleanly defined sematics)
would be certainly nice to have.
> But I get way ahead of myself. First we need to get these last few bits
> and pieces in place to accelerate (non --preserve-merges) rebase -i.
But it can wait, right.
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests
From: Junio C Hamano @ 2016-09-01 22:52 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: Johannes Schindelin, Jeff King, Git
In-Reply-To: <CAGdFq_h3UuW7wX0-=SuS22mX_C086HRZZ=i1sYVya80dd+qMYQ@mail.gmail.com>
Sverre Rabbelier <srabbelier@gmail.com> writes:
>>> I can't really recall, but I think it may have been related to me
>>> doing something like this:
>>> 1. Make a change, and start running tests (this takes a long time)
>>> 2. Notice a failure, start fixing it, leave tests running to find
>>> further failures
>>> 3. Finish fix, first tests are still running, start another run in a
>>> new terminal (possibly of just the one failed test I was fixing) to
>>> see if the fix worked.
>>>
>>> Without the pid, the second run would clobber the results from the first run.
>>>
>> Would present-you disagree with stripping off the -<pid> suffix, based on
>> your recollections?
>
> No objections, I think it should be fine. If anyone uncovers a
> particularly compelling reason later on, it's only a commit away :).
OK, especially with the earlier observation made by Peff in the log
message:
... we can see that other files we write to test-results (like
*.exit and *.out) do _not_ have the PID included. So the
presence of the PID does not meaningfully allow one to store the
results from multiple runs anyway.
even if we wanted to, keeping the current code with suffix is not
sufficient, so I suspect it won't be just "a commit" away, but we
should be able to lose it for now. Hopefully that would help making
Dscho's "what are the failed tests?" logic simpler.
Thanks.
^ permalink raw reply
* Re: [PATCH 13/22] sequencer: remember the onelines when parsing the todo file
From: Jakub Narębski @ 2016-09-01 22:46 UTC (permalink / raw)
To: Johannes Schindelin, Junio C Hamano; +Cc: git
In-Reply-To: <alpine.DEB.2.20.1609011052260.129229@virtualbox>
W dniu 01.09.2016 o 11:37, Johannes Schindelin pisze:
> On Wed, 31 Aug 2016, Junio C Hamano wrote:
>> Jakub Narębski <jnareb@gmail.com> writes:
>>
>>>> diff --git a/sequencer.c b/sequencer.c
>>>> index 06759d4..3398774 100644
>>>> --- a/sequencer.c
>>>> +++ b/sequencer.c
>>>> @@ -709,6 +709,8 @@ static int read_and_refresh_cache(struct replay_opts *opts)
>>>> struct todo_item {
>>>> enum todo_command command;
>>>> struct commit *commit;
>>>> + const char *arg;
>>>> + int arg_len;
>>>
>>> Why 'arg', and not 'oneline', or 'subject'?
>>> I'm not saying it is bad name.
>>
>> I am not sure what the "commit" field of type "struct commit *" is
>> for. It is not needed until it is the commit's turn to be picked or
>> reverted; if we end up stopping in the middle, parsing the commit
>> object for later steps will end up being wasted effort.
>
> No, it won't be wasted effort, as we *validate* the todo script this way.
> And since we may very well need the info later (most rebases do not fail
> in the middle), we store it, too.
The question was (I think) whether we should do eager parsing of
commits, or whether we can do lazy parsing by postponing full parsing
"until it is the commit's turn to be picked or reverted", and possibly
when saving todo file.
I wonder how probable is situation where we save instruction sheet
for interactive rebase, with shortened SHA-1, and during rebase
shortened SHA-1 stops being unambiguous...
> [...] after parsing the todo_list, we will
> have to act on the information contained therein. For example we will have
> to cherry-pick some of the indicated commits (requiring a struct commit *
> for use in do_pick_commit()). Another example: we may need to determine
> the oneline for use in fixup!/squash! reordering.
>
> So: keeping *that* aspect of the previous todo_list parsing, i.e. store a
> pointer to the already-parsed commit, is the right thing to do.
The above probably means that eager eval is better
Best,
--
Jakub Narębski
^ permalink raw reply
* Re: `make profile-install` fails in 2.9.3
From: Junio C Hamano @ 2016-09-01 22:38 UTC (permalink / raw)
To: Jeff King
Cc: Thomas Gummerer, Jan Keromnes, git, Ingo Brückl,
Edward Thomson
In-Reply-To: <20160901222015.wqhrerudnwddbyhz@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Yeah, I had a similar thought, but it just feels so hacky. Is there
> anything wrong with making this completely separate from the content
> update. I.e., just applying the pathspec to the index as a separate step
> and adding "+x" to each entry?
>
> This really is just a more convenient interface around "update-index
> --chmod", isn't it? We should be able to do the same thing it does.
Sure, the simplest and the most straight-forward way may look dumb,
but it would be the safest.
^ permalink raw reply
* Re: [PATCH 10/22] sequencer: avoid completely different messages for different actions
From: Jakub Narębski @ 2016-09-01 22:33 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Jiang Xin
In-Reply-To: <alpine.DEB.2.20.1609010950150.129229@virtualbox>
Hello Johannes
W dniu 01.09.2016 o 09:52, Johannes Schindelin pisze:
> On Wed, 31 Aug 2016, Jakub Narębski wrote:
>> CC-ed to Jiang Xin, L10N coordinator.
>> W dniu 29.08.2016 o 10:05, Johannes Schindelin pisze:
[...]
>>> - /* Different translation strings for cherry-pick and revert */
>>> - if (opts->action == REPLAY_PICK)
>>> - error(_("Your local changes would be overwritten by cherry-pick."));
>>> - else
>>> - error(_("Your local changes would be overwritten by revert."));
>>> + error(_("Your local changes would be overwritten by %s."),
>>> + action_name(opts));
>>
>> If I understand it correctly, it would make "revert" or "cherry-pick"
>> untranslated part of error message. You would need to use translation
>> on the result with "_(action_name(opts))", you would have to mark
>> todo_command_strings elements for gettext lexicon with N_(...).
>>
>> I am rather against this change (see also below).
>
> Okay.
>
> Unfortunately, I have to focus on the correctness of the code at the
> moment (and Git for Windows does ship *without* translations for the time
> being anyway, mostly to save on space, but also because users complained).
Users complained about having translations, or not having easy way to
switch them or switch them off?
>
> So I will take care of this after v2.10.0.
>
> For the record, how is this supposed to be handled, in particular when I
> introduce a new action whose action_name(opts) will be "rebase -i"? Do I
> really need to repeat myself three times?
I think you should be able to mark strings to be translated,
without translating them at the time of definition,
static const char *todo_command_strings[] = {
N_("pick"),
N_("revert")
};
then translate at the point of use
error(_("Your local changes would be overwritten by %s."),
_(action_name(opts)));
I assume that action_name(opts) returns one of todo_command_strings.
If not, there should be array with possible actions.
Assuming that such lego l10n is preferable to multiple translations,
more free-formt.
--
Jakub Narębski
^ permalink raw reply
* Re: Should "git symbolic-ref -d HEAD" be forbidden?
From: Junio C Hamano @ 2016-09-01 22:31 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20160901211907.iivokwu3yjuxz3qf@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Thu, Sep 01, 2016 at 02:08:08PM -0700, Junio C Hamano wrote:
>
>> I think we should.
>>
>> t1401 expects to be able to, but if you really do it:
>>
>> $ cd /tmp
>> $ git init throwaway
>> $ cd throwaway
>> $ git symbolic-ref -d HEAD
>>
>> the setup machinery considers that you are no longer in a working
>> tree that is controlled by a repository at .git/ because .git/ is
>> no longer a valid repository, so you cannot even do
>>
>> $ git symbolic-ref HEAD refs/heads/master
>>
>> to recover.
>
> Yes, I think we should, too. The same reasoning from afe5d3d (symbolic
> ref: refuse non-ref targets in HEAD, 2009-01-29) applies.
-- >8 --
Subject: symbolic-ref -d: do not allow removal of HEAD
If you delete the symbolic-ref HEAD from a repository, Git no longer
considers it valid, and even "git symbolic-ref HEAD refs/heads/master"
would not be able to recover from that state.
In the spirit similar to afe5d3d5 ("symbolic ref: refuse non-ref
targets in HEAD", 2009-01-29), forbid removal of HEAD.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
I decided against it for now for no good reason, other than I am a
bit superstitious, but it may be a good idea to move these safety
checks to delete_ref() and create_symref() in the longer term.
builtin/symbolic-ref.c | 2 ++
t/t1401-symbolic-ref.sh | 19 ++++++++++++-------
2 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
index 9c29a64..96eed94 100644
--- a/builtin/symbolic-ref.c
+++ b/builtin/symbolic-ref.c
@@ -56,6 +56,8 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
ret = check_symref(argv[0], 1, 0, 0);
if (ret)
die("Cannot delete %s, not a symbolic ref", argv[0]);
+ if (!strcmp(argv[0], "HEAD"))
+ die("deleting '%s' is not allowed", argv[0]);
return delete_ref(argv[0], NULL, REF_NODEREF);
}
diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh
index ca3fa40..5c30f94 100755
--- a/t/t1401-symbolic-ref.sh
+++ b/t/t1401-symbolic-ref.sh
@@ -33,18 +33,23 @@ test_expect_success 'symbolic-ref refuses bare sha1' '
'
reset_to_sane
-test_expect_success 'symbolic-ref deletes HEAD' '
- git symbolic-ref -d HEAD &&
+test_expect_success 'HEAD cannot be removed' '
+ test_must_fail git symbolic-ref -d HEAD
+'
+
+test_expect_success 'symbolic-ref can be deleted' '
+ git symbolic-ref NOTHEAD refs/heads/foo &&
+ git symbolic-ref -d NOTHEAD &&
test_path_is_file .git/refs/heads/foo &&
- test_path_is_missing .git/HEAD
+ test_path_is_missing .git/NOTHEAD
'
reset_to_sane
-test_expect_success 'symbolic-ref deletes dangling HEAD' '
- git symbolic-ref HEAD refs/heads/missing &&
- git symbolic-ref -d HEAD &&
+test_expect_success 'symbolic-ref can delete dangling symref' '
+ git symbolic-ref NOTHEAD refs/heads/missing &&
+ git symbolic-ref -d NOTHEAD &&
test_path_is_missing .git/refs/heads/missing &&
- test_path_is_missing .git/HEAD
+ test_path_is_missing .git/NOTHEAD
'
reset_to_sane
^ permalink raw reply related
* Re: Are --first-parent and --ancestry-path compatible rev-list options?
From: Junio C Hamano @ 2016-09-01 22:20 UTC (permalink / raw)
To: Philip Oakley; +Cc: Git List
In-Reply-To: <1BC836ACA8C741AD9732122B3B198752@PhilipOakley>
"Philip Oakley" <philipoakley@iee.org> writes:
> The user question was, given a commit 'J', and a future commit 'H'
> (typically a branch tip such as 'master'), find those commits that are
> :
> A) merges
> B) on the first parent DAG chain of the future commit 'H'
> C) children of the given commit 'J'
The answer then is that there is no such single step operation.
In the picture, if D or E were a merge from a side branch that does
not have anything to do with 'J', "log --first-parent --merges" will
not exclude it (i.e. C won't be fulfilled by --first-parent --merges).
^ permalink raw reply
* Re: `make profile-install` fails in 2.9.3
From: Jeff King @ 2016-09-01 22:20 UTC (permalink / raw)
To: Junio C Hamano
Cc: Thomas Gummerer, Jan Keromnes, git, Ingo Brückl,
Edward Thomson
In-Reply-To: <xmqqh99zuuyh.fsf@gitster.mtv.corp.google.com>
On Thu, Sep 01, 2016 at 03:16:38PM -0700, Junio C Hamano wrote:
> Which means that piggybacking this on the "run 'git diff' limited to
> the pathspec to find the paths that needs updating" logic usually
> done in "git add" can not be reused [*1*].
>
> What was I thinking while reviewing the patch X-<. Sigh.
>
>
> [Footnote]
>
> *1* I guess we _could_, by first flipping all the regular file
> blob's executable bit for paths that are inside the pathspec and
> then by running "git diff" against that modified index, limited
> to the pathspec, to find the paths that need to be added.
>
> It sounds ugly, but may conceptually be cleaner. We first start
> from an ideal end-result, and then re-hash what needs to be
> updated to match the ideal.
Yeah, I had a similar thought, but it just feels so hacky. Is there
anything wrong with making this completely separate from the content
update. I.e., just applying the pathspec to the index as a separate step
and adding "+x" to each entry?
This really is just a more convenient interface around "update-index
--chmod", isn't it? We should be able to do the same thing it does.
-Peff
^ permalink raw reply
* Re: `make profile-install` fails in 2.9.3
From: Junio C Hamano @ 2016-09-01 22:16 UTC (permalink / raw)
To: Jeff King
Cc: Thomas Gummerer, Jan Keromnes, git, Ingo Brückl,
Edward Thomson
In-Reply-To: <20160901215810.ez47lqwmfmahyvc7@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Yeah, I think we should _always_ act on the --chmod, no matter if the
> file is racy or not, or whether it has a content change or not. I.e.,
> the race is not the problem, but rather the behavior of 4e55ed32. Your
> second proposal there sounds more like the right approach.
Yeah, you two are absolutely right. The second "git add --chmod=+x"
in
$ git add .
$ git add --chmod=+x .
should still find _all_ the non-executable paths and flip their
executable bit in the index, making them all up-to-date in the
index.
Which means that piggybacking this on the "run 'git diff' limited to
the pathspec to find the paths that needs updating" logic usually
done in "git add" can not be reused [*1*].
What was I thinking while reviewing the patch X-<. Sigh.
[Footnote]
*1* I guess we _could_, by first flipping all the regular file
blob's executable bit for paths that are inside the pathspec and
then by running "git diff" against that modified index, limited
to the pathspec, to find the paths that need to be added.
It sounds ugly, but may conceptually be cleaner. We first start
from an ideal end-result, and then re-hash what needs to be
updated to match the ideal.
^ permalink raw reply
* Re: [PATCH v13 06/14] apply: make it possible to silently apply
From: Stefan Beller @ 2016-09-01 16:57 UTC (permalink / raw)
To: Christian Couder
Cc: git@vger.kernel.org, Junio C Hamano, Jeff King,
Ævar Arnfjörð Bjarmason, Karsten Blees,
Nguyen Thai Ngoc Duy, Eric Sunshine, Ramsay Jones, Johannes Sixt,
René Scharfe, Stefan Naewe, Christian Couder
In-Reply-To: <CAP8UFD3RE6L17nQmvZTkx6wycsdG7EA9eM=tYQD8nrS445zg1w@mail.gmail.com>
On Thu, Sep 1, 2016 at 1:01 AM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Thu, Sep 1, 2016 at 12:07 AM, Stefan Beller <sbeller@google.com> wrote:
>>> Printing on stdout, and calls to warning() or error() are not
>>> taken care of in this patch, as that will be done in following
>>> patches.
>>
>>> - if (state->apply_verbosely)
>>> + if (state->apply_verbosity > verbosity_normal)
>>> error(_("while searching for:\n%.*s"),
>>> (int)(old - oldlines), oldlines);
>>
>> But this is an error(..) ?
>
> Do you mean that it was a bug in the original code to print this error
> only in verbose mode?
Oh never mind.
I meant to point out the inconsistency between the commit message, that
said: "error() are not taken care of in this patch" and modifying a
condition for
an error call. However we need to fix them as you renamed apply_verbosely
to apply_verbosity and made it an enum. So I spoke too early.
>
> Anyway I don't think such a refactoring is needed.
great :)
^ permalink raw reply
* Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests
From: Sverre Rabbelier @ 2016-09-01 16:57 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, Jeff King, Git
In-Reply-To: <alpine.DEB.2.20.1609011027210.129229@virtualbox>
On Thu, Sep 1, 2016 at 1:27 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Wed, 31 Aug 2016, Sverre Rabbelier wrote:
>> On Wed, Aug 31, 2016 at 3:36 AM Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>> > On Tue, 30 Aug 2016, Junio C Hamano wrote:
>> > > Jeff King <peff@peff.net> writes:
>> > > > Hmm, interesting. Your approach seems reasonable, but I have to wonder
>> > > > if writing the pid in the first place is sane.
>> > > >
>> > > > I started to write up my reasoning in this email, but realized it was
>> > > > rapidly becoming the content of a commit message. So here is that
>> > > > commit.
>> > >
>> > > Sounds sensible; if this makes Dscho's "which ones failed in the
>> > > previous run" simpler, that is even better ;-)
>> >
>> > I did not have the time to dig further before now. There must have been a
>> > good reason why we append the PID.
>> >
>> > Sverre, you added that code in 2d84e9f (Modify test-lib.sh to output stats
>> > to t/test-results/*, 2008-06-08): any idea why the -<pid> suffix was
>> > needed?
>>
>> I can't really recall, but I think it may have been related to me
>> doing something like this:
>> 1. Make a change, and start running tests (this takes a long time)
>> 2. Notice a failure, start fixing it, leave tests running to find
>> further failures
>> 3. Finish fix, first tests are still running, start another run in a
>> new terminal (possibly of just the one failed test I was fixing) to
>> see if the fix worked.
>>
>> Without the pid, the second run would clobber the results from the first run.
>>
>>
>> If only past-me was more rigorous about writing good commit messages :P.
>
> :-)
>
> Would present-you disagree with stripping off the -<pid> suffix, based on
> your recollections?
No objections, I think it should be fine. If anyone uncovers a
particularly compelling reason later on, it's only a commit away :).
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [PATCH 09/22] sequencer: completely revamp the "todo" script parsing
From: Jakub Narębski @ 2016-09-01 22:05 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1609010836010.129229@virtualbox>
Hello Johannes,
W dniu 01.09.2016 o 09:49, Johannes Schindelin pisze:
> On Wed, 31 Aug 2016, Jakub Narębski wrote:
>> W dniu 29.08.2016 o 10:05, Johannes Schindelin pisze:
[...]
>> Hmmm... commit_list is, as defined in commit.h, a linked list.
>
> That is the most prominent reason why the rest is not a mindless
> conversion from commit_list to todo_list.
>
> And we need todo_list as an array, because we need to be able to peek, or
> even move, backwards from the current command.
>
>> Here todo_list uses growable array implementation of list. Which
>> is I guess better on current CPU architecture, with slow memory,
>> limited-size caches, and adjacency prefetching.
>
> That is not the reason that an array is used here. The array allows us
> much more flexibility.
It would be nice if this reasoning (behind the change from linked list
to growable array) was mentioned in appropriate commit message, and
perhaps also in the cover letter for the series. It is IMVHO quite
important information (that you thought obvious).
>
> One of the major performance improvements will come at the very end, for
> example: the reordering of the fixup!/squash! lines. And that would be a
> *major* pain to do if the todo_list were still a linked list.
Actually deletion from and insertion into single linked list are
not that hard, and O(1) after finding place, O(N) with finding
included. Moving elements in array is O(N),... and arguably a bit
simpler - but at high level, with appropriate primitives, they are
about the same.
Yes, array is easier for permutation and reordering.
>>> +struct todo_item *append_todo(struct todo_list *todo_list)
>>
>> Errr... I don't quite understand the name of this function.
>> What are you appending here to the todo_list?
>
> A new item.
>
>> Compare string_list_append() and string_list_append_nodup(),
>> where the second parameter is item to append.
>
> Yes, that is correct. In the case of a todo_item, things are a lot more
> complicated, though. Some of the values have to be determined tediously
> (such as the offset and length of the oneline after the "pick <oid>"
> command). I just put those values directly into the newly allocated item,
> is all.
I would expect sth_append command to take a list (or other collection),
an element, and return [modified] collection with the new element added.
Such API would require temporary variable in caller and memcopy in the
sth_append() function.
This is not it. It creates a new element, expanding a list (a collection),
and then expose this element. Which spares us memcopy... on non-critical
path.
I don't know how to name operation "grow list and return new element".
But "append" it is not.
>>> + ALLOC_GROW(todo_list->items, todo_list->nr + 1, todo_list->alloc);
>>> + return todo_list->items + todo_list->nr++;
>>> }
>>>
>>> -static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts)
>>> +static int parse_insn_line(struct todo_item *item, const char *bol, char *eol)
>>
>> Why the change of return type?
>
> Because it makes no sense to return a commit here because not all commands
> are about commits (think rebase -i's `exec`). It makes tons of sense to
> return an error condition, though.
All right.
I have not checked / not remember what caller of parse_insn_line() used
it's return value for. I guess that we save it into todo_item, moving
that operation from caller to callee.
>> Why now struct todo_item is first when struct replay_opts was last?
>
> Those play very, very different roles.
>
> The opts parameter used to provide parse_insn_line() with enough
> information to complain loudly when the overall command was not identical
> to the parsed command.
>
> The item parameter is a receptacle for the parsed data. It will contain
> the pointer to the commit that was previously returned, if any. But it
> will also contain much more information, such as the command, the oneline,
> the offset in the buffer, etc etc
>
> So "opts" was an "in" parameter while "item" is an "out" one. Apples and
> oranges.
All right. Good explanation. And the one that is too low-level
detail to put in the commit message, I think.
>>> - end_of_object_name = bol + strcspn(bol, " \t\n");
>>> + end_of_object_name = (char *) bol + strcspn(bol, " \t\n");
>>
>> Why is this cast needed?
>
> Because bol is a "const char *" and we need to put "NUL" temporarily to
> *end_of_object_name:
Would compiler complain without this const'ness-stripping cast?
>
>>> saved = *end_of_object_name;
>>> *end_of_object_name = '\0';
>>> status = get_sha1(bol, commit_sha1);
>>> *end_of_object_name = saved;
>
> Technically, this would have made a fine excuse to teach get_sha1() a mode
> where it expects a length parameter instead of relying on a NUL-terminated
> string.
>
> Practically, such fine excuses cost me months in this rebase--helper
> project already, and I need to protect my time better.
Put it in TODO list (and perhaps add a TODO comment) ;-).
BTW. open-source produces better software, and development bandwidth
is good, but the latency :-((((
>
>>> - /*
>>> - * Verify that the action matches up with the one in
>>> - * opts; we don't support arbitrary instructions
>>> - */
>>> - if (action != opts->action) {
>>> - if (action == REPLAY_REVERT)
>>> - error((opts->action == REPLAY_REVERT)
>>> - ? _("Cannot revert during another revert.")
>>
>> Errr... could the above ever happen? Namely
>>
>> action != opts->action && action == REPLAY_REVERT && opts->action == REPLAY_REVERT
>>
>> Surely not.
>
> Your reply pointed to the very circumstance when this may happen: `git
> cherry-pick --continue` after an interrupted `git revert`.
I was talking about "Cannot revert during another revert." and
"Cannot cherry-pick during another cherry-pick." errors, which can
never happen because of the outermost if.
x == A && y == A && x != Y
can never happen, because equality is transitive^*
x == A && y == A => x == y
*) except NaN (NaN != NaN), but we don't deal with floating point here.
From those error messages it looks like they were originally intended
to prevent from starting another revert or cherry-pick if sequencer
based operation is already in progress (to not stomp on the latter
internal state). But for that it would need to examine the
opts->subcommand if it is REPLAY_NONE (or equivalent), check that
sequencer file already exists, and read it to fond which operation
is in progress.
This is about checking that command in todo-list agrees with
the git command used (we probably checked that it is --continue,
or maybe --skip). But the error message does not spell that;
it is misleading.
[Nb. git-blame shows Vasco Almeida as author of those lines]
>
> But then, I remove that code here, so I should not try to defend it.
>
But that was / is another reason for removing code: it is slightly
wrong.
>>> - : _("Cannot revert during a cherry-pick."));
>>> - else
>>> - error((opts->action == REPLAY_REVERT)
>>> - ? _("Cannot cherry-pick during a revert.")
>>> - : _("Cannot cherry-pick during another cherry-pick."));
>>> - return NULL;
>>> - }
>>
>> Anyway, while it is / would be a good idea to prevent starting any
>> sequencer-based command (cherry-pick, revert, soon rebase -i) when
>> other command is in progress (cherry-pick, revert, soon rebase -i).
>> That is, if cherry-pick / revert waits for user action, you cannot
>> run another cherry-pick or revert.
>>
>> Which I guess the above code was not about...
>
> It was about that, though.
No it was not, see the reasoning above.
>
> It went about it in a pretty round-about way: opts->action comes from the
> name of the command ("was I called as `git revert` or `git cherry-pick`?")
> and action comes from the todo script, which was assumed to be written by
> a previous run of the sequencer, using the then-current value of
> opts->action.
It did different check, that we continue with the same command
as we began with.
>
> So it wrote that command into *every single line* of the todo script, *for
> the sole purpose* of verifying that it was the same action when running
> via --continue.
>
> As I said earlier, I would not complain at all if an interrupted `git
> revert` could be continued via `git cherry-pick --continue`.
>
> If that is not desirable, I can reintroduce that overzealous check, but
> that will have to wait until after v2.10.0. And it would require an
> argument that convinces me.
One argument is that you can add options to --continue, and among
options for cherry-pick, revert and rebase -i, there are options
that apply only to some of them. So we need at least decide what
to do if we started rebase, and try to continue with cherry-pick:
do we accept rebase-only options? What we do with cherry-pick-only
options, that rebase does not understand?
It is easier to just check that revert is continued with revert
(by checking command in todo file), cherry-pick with cherry-pick
(same), rebase -i with rebase (by checking another file), and
am with am.
>
>>> + item = append_todo(todo_list);
>>
>> A better name, in my personal option, would be
>>
>> + item = todo_list_next(todo_list);
>>
>> Or todo_next(todo_list).
>
> That sounds more like a function that performs the next command in the
> todo_list.
>
> While I agree that naming is hard, I still think that `append_todo()` with
> the todo_list as single parameter and returning a todo_item is pretty much
> self-explanatory: it appends a new item to the todo_list and returns a
> pointer to it.
It creates new item in the todo_list at the end, or it grows todo_list,
but the function does not append anything...
[...]
>>> - if (strbuf_read(&buf, fd, 0) < 0) {
>>> + if (strbuf_read(&todo_list->buf, fd, 0) < 0) {
>>> close(fd);
>>> - strbuf_release(&buf);
>>
>> A question: when is todo_list->buf released?
>
> Why, I am glad you asked! It is released in todo_list_release(), called at
> the end e.g. of sequencer_continue().
All right. I could have guessed that.
>>> -static int walk_revs_populate_todo(struct commit_list **todo_list,
>>> +static int walk_revs_populate_todo(struct todo_list *todo_list,
>>> struct replay_opts *opts)
>>> {
>>> + enum todo_command command = opts->action == REPLAY_PICK ?
>>> + TODO_PICK : TODO_REVERT;
>>> struct commit *commit;
>>> - struct commit_list **next;
>>>
>>> if (prepare_revs(opts))
>>> return -1;
>>>
>>> - next = todo_list;
>>> - while ((commit = get_revision(opts->revs)))
>>> - next = commit_list_append(commit, next);
>>> + while ((commit = get_revision(opts->revs))) {
>>> + struct todo_item *item = append_todo(todo_list);
>>> + const char *commit_buffer = get_commit_buffer(commit, NULL);
>>
>> I see that you are creating todo file contents while walking revision list,
>> something that was left for later in current / previous implementation
>> of the sequencer...
>
> Not really. This function was always about generating a todo_list. It just
> did not format it yet.
>
> With the change of keeping the original formatting of the todo script
> instead of re-formatting it in save_todo(), this function now has to
> format the todo_list itself.
So the commit parsing was moved from save_todo(), which contrary to
the name also re-generated todo list, to walk_revs_populate_todo().
I guess it is the same callchain.
Additional question, answer to which should address Junio's complaint
about early/eager commit parsing (if I understand it correctly):
do information from parsing commit is needed for creating a new
commit, i.e. doing a pick or revert? If not, then perhaps parsing
only those commits that are left, at the time of saving todo file
(with probably less commit that what we have started with), would
be better?
[...]
>>> -static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)
>>> +static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
>>> {
>>> static struct lock_file todo_lock;
>>> - struct strbuf buf = STRBUF_INIT;
>>> - int fd;
>>> + const char *todo_path = get_todo_path(opts);
>>> + int next = todo_list->current, offset, fd;
>>
>> The "next = todo_list->current" looks a bit strange.
>
> Depending whether we need rebase -i processing or revert/cherry-pick's
> slightly different one, the "current" position points to the next one
> already...
>
>> Also, we do not change todo_list->current, we use it in one place, so it
>> can be used directly without help of temporary / helper variable. But
>> that is just my personal opinion.
>
> No, it has nothing to do with opinion. It prepares the code to keep it
> readable even when REPLAY_INTERACTIVE_REBASE is introduced.
Ah, all right.
[...]
>> and this should be done in one of earlier patches, isn't it?
>
> No. I deliberately skipped save_todo() from "future-proofing" as I planned
> to rewrite it anyway. There is no point in future-proofing something you
> are going to toss in a minute.
>
All right. Though I wonder if it should not be mentioned in
the commit message of said previous patch (though I have missed
that not all sites were "future-proofed" in review; I'm sorry).
>>> - if (commit_lock_file(&todo_lock) < 0) {
>>> - strbuf_release(&buf);
>>> - return error(_("Error wrapping up %s."), git_path_todo_file());
>>> - }
>>> - strbuf_release(&buf);
>>> + if (commit_lock_file(&todo_lock) < 0)
>>> + return error(_("Error wrapping up %s."), todo_path);
>>
>> Note: this is unrelated change, but we usually put paths in quotes, like this
>>
>> + return error(_("Error wrapping up '%s'."), todo_path);
>>
>> (in this and earlier error message), so that paths containing spaces show
>> correctly and readably to the user. Though this possibly is not a problem
>> for this path.
>
> Right.
>
>> Also, how user is to understand "wrapping up"?
>
> The same as before: the removed lines already had the error message,
> missing the quotes, too.
>
> Don't get me wrong: I am a big fan of consistency, and I wish that Git's
> source code had more of it. So I would love to see a patch series that
> makes all error messages consistently reporting paths enclosed in single
> quotes.
>
> I am also a big fan of the separation of concerns, though. And this patch
> series' concern is consistency *with the existing code*.
>
> So I won't change the error message that I inherited at this point.
All right, I can understand that. But I think it won't cost
much to do _while at it_ adding of quotes around pathnames in
error messages, where you notice this problem.
>>> static int single_pick(struct commit *cmit, struct replay_opts *opts)
>>> {
>>> setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
>>> - return do_pick_commit(cmit, opts);
>>> + return do_pick_commit(opts->action == REPLAY_PICK ?
>>> + TODO_PICK : TODO_REVERT, cmit, opts);
>>
>> The ternary conditional operator here translates one enum to other enum,
>> isn't it?
>
> Well, almost. Please note that the enum will receive a new value in the
> sequencer-i patch series. And there is no equivalent todo_command for
> REPLAY_INTERACTIVE_REBASE.
All right. And casting one enum to other, relying on the same
order, is tricky to the extreme and brittle.
>
> Thanks for the review!
You are welcome.
Best,
--
Jakub Narębski
^ permalink raw reply
* Re: `make profile-install` fails in 2.9.3
From: Jeff King @ 2016-09-01 21:58 UTC (permalink / raw)
To: Thomas Gummerer; +Cc: Jan Keromnes, git, Ingo Brückl, Edward Thomson
In-Reply-To: <20160901200700.GA8254@hank>
On Thu, Sep 01, 2016 at 09:07:00PM +0100, Thomas Gummerer wrote:
> > Related problem: `t3700-add.sh` currently fails in 2.9.3. I can
> > provide more debug information if you don't already know this problem.
>
> I noticed this problem as well, when I'm compiling with USE_NSEC = 1
> in my config.mak.
I can replicate this even without USE_NSEC with my stress-tester[1].
That makes sense why it would show up with the profiling run; git runs
slower and therefore increases the chances of crossing the 1-second
boundary and losing the race.
[1] https://github.com/peff/git/blob/meta/stress
> Tracking this problem down a bit, it happens because the --chmod=[+-]x
> option introduced in 4e55ed32 ("add: add --chmod=+x / --chmod=-x
> options") only works if the file on disk is modified. When the test
> was changed to work on one single file, instead of doing chmod=+x on
> one file and chmod=-x on another file in b38ab197c ("t3700: merge two
> tests into one"), this test started breaking when the mtime of the
> file and the index file weren't the same (in other words, if the file
> was not racily clean and thus was not smudged).
That certainly sounds buggy. A less racy way of verifying this is just:
# guarantee not-racy state
echo content >file
test-chmtime -60 file
git add file
# now check --chmod; file will still be 100644!
git add --chmod=+x file
git ls-files -s
> One possible fix for the test is to smudge the entry as showed below,
> though I'm not sure it's the right fix. The other way I can think of
> is to change the file in the index regardless of whether the file was
> changed in some other way before issuing the git add command, as that
> might fit the user expectation better. Thoughts?
Yeah, I think we should _always_ act on the --chmod, no matter if the
file is racy or not, or whether it has a content change or not. I.e.,
the race is not the problem, but rather the behavior of 4e55ed32. Your
second proposal there sounds more like the right approach.
-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