* Re: [PATCH v2 21/25] sequencer: refactor write_message()
From: Johannes Schindelin @ 2016-10-05 13:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, git, Jakub Narębski
In-Reply-To: <xmqqfup1nf3u.fsf@gitster.mtv.corp.google.com>
Hi Junio & Hannes,
On Thu, 15 Sep 2016, Junio C Hamano wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
>
> > Am 11.09.2016 um 12:55 schrieb Johannes Schindelin:
> >> -static int write_message(struct strbuf *msgbuf, const char *filename)
> >> +static int write_with_lock_file(const char *filename,
> >> + const void *buf, size_t len, int append_eol)
> >> {
> >> static struct lock_file msg_file;
> >>
> >> int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);
> >> if (msg_fd < 0)
> >> return error_errno(_("Could not lock '%s'"), filename);
> >> - if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
> >> - return error_errno(_("Could not write to %s"), filename);
> >> - strbuf_release(msgbuf);
> >> + if (write_in_full(msg_fd, buf, len) < 0)
> >> + return error_errno(_("Could not write to '%s'"), filename);
> >> + if (append_eol && write(msg_fd, "\n", 1) < 0)
> >> + return error_errno(_("Could not write eol to '%s"), filename);
> >> if (commit_lock_file(&msg_file) < 0)
> >> return error(_("Error wrapping up %s."), filename);
> >>
> >> return 0;
> >> }
> >
> > The two error paths in the added lines should both
> >
> > rollback_lock_file(&msg_file);
> >
> > , I think. But I do notice that this is not exactly new, so...
>
> It may not be new for this step, but overall the series is aiming to
> libify the stuff, so we should fix fd and lockfile leaks like this
> as we notice them.
Makes sense, even for the final commit_lock_file().
Ciao,
Dscho
^ permalink raw reply
* Re: [musl] Re: Regression: git no longer works with musl libc's regex impl
From: Szabolcs Nagy @ 2016-10-05 13:01 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Rich Felker, Jeff King, git, musl
In-Reply-To: <alpine.DEB.2.20.1610051250080.35196@virtualbox>
* Johannes Schindelin <Johannes.Schindelin@gmx.de> [2016-10-05 13:17:49 +0200]:
> I had a brief look at the source code (you use backtracking... hopefully
> nobody uses musl to parse regular expressions from untrusted, or
> inexperienced, sources [*1*]), and it seems that the regex code might
does git use BRE?
a conforming BRE implementation has to use back tracking
if the pattern has back references.
usually ERE implementations may also use back tracking
since they support back references as an extension.
musl does not support this extension (and many others) so
it never uses back tracking for ERE matches, note however
that match complexity and memory usage of a conforming
ERE implementation is still exponential in pattern
length because of repetition counts.
^ permalink raw reply
* Re: [PATCH v2 17/25] sequencer: support amending commits
From: Johannes Schindelin @ 2016-10-05 12:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <xmqq8tuwztpd.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 12 Sep 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > This teaches the sequencer_commit() function to take an argument that
> > will allow us to implement "todo" commands that need to amend the commit
> > messages ("fixup", "squash" and "reword").
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > sequencer.c | 6 ++++--
> > sequencer.h | 2 +-
> > 2 files changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/sequencer.c b/sequencer.c
> > index 6e9732c..60b522e 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -485,7 +485,7 @@ static char **read_author_script(void)
> > * author metadata.
> > */
> > int sequencer_commit(const char *defmsg, struct replay_opts *opts,
> > - int allow_empty, int edit)
> > + int allow_empty, int edit, int amend)
> > {
> > char **env = NULL;
> > struct argv_array array;
> > @@ -514,6 +514,8 @@ int sequencer_commit(const char *defmsg, struct replay_opts *opts,
> > argv_array_push(&array, "commit");
> > argv_array_push(&array, "-n");
> >
> > + if (amend)
> > + argv_array_push(&array, "--amend");
> > if (opts->gpg_sign)
> > argv_array_pushf(&array, "-S%s", opts->gpg_sign);
> > if (opts->signoff)
> > @@ -786,7 +788,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
> > }
> > if (!opts->no_commit)
> > res = sequencer_commit(opts->edit ? NULL : git_path_merge_msg(),
> > - opts, allow, opts->edit);
> > + opts, allow, opts->edit, 0);
> >
> > leave:
> > free_message(commit, &msg);
>
> Hmm, this is more about a comment on 18/25, but I suspect that
> "amend" or any opportunity given to the user to futz with the
> contents in the editor invites a wish for the result to be treated
> with stripspace.
The point of separating the cleanup_commit_message from the edit flag is
to allow final fixup commands to stripspace, even without letting the user
edit the message.
So while you are correct that "edit != 0" should imply
"cleanup_commit_message != 0", I would rather keep that explicit.
> No existing callers use "amend" to call this function, so there is
> no change in behaviour, but at the same time, we do not have enough
> information to see if 'amend' should by default toggle cleanup.
It should not. Non-final fixup/squash commands *need* to keep the
comment lines.
Keeping things explicit makes it easier to understand the flow, methinks.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v2 19/25] sequencer: remember do_recursive_merge()'s return value
From: Johannes Schindelin @ 2016-10-05 12:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <xmqqh99kzua9.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 12 Sep 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > The return value of do_recursive_merge() may be positive (indicating merge
> > conflicts), so let's OR later error conditions so as not to overwrite them
> > with 0.
>
> Are the untold assumptions as follows?
>
> - The caller wants to act on positive (not quite an error), zero
> (success) or negative (error);
>
> - do_recursive_merge() can return positive (success with reservation),
> zero or negative, and the call to it would return immediately if it got
> negative;
>
> - all other functions that come later may return either zero or
> negative, and never positive;
>
> - Hence the caller can be assured that "res" being positive can only
> mean do_recursive_merge() succeeded with reservation and everything
> else succeeded.
More or less. The idea is that there are problems with the merge, and
there are fatal errors.
Alex and I chose to stay close to the original Python implementation when
we decided that merge_trees() should return 1 for success and 0 for
failure. In hindsight, I regret that, as it disagrees with the C
convention to return 0 for success. However, it would have been yet
another mistake to return -1 in case of merge conflicts: the function did
not have a problem (such as out-of-memory or some such).
I can only guess that the do_recursive_merge() function tried to undo the
damage by reversing the logic: 0 for success, 1 for unclean merge. It is
at least more in line with the C convention.
So much so, in fact, that we can still use negative values to indicate
fatal errors that should be handled accordingly.
> This can be extended if the only thing the caller cares about is
> positive/zero/negative and it does not care what exact positive
> value it gets--in such a case, we can loosen the condition on the
> return values from other functions whose return values are OR'ed
> together; they may also return positive to signal the same "not
> quite an error", i.e. updating the latter two points to
>
> - all other functions that come later can return positive (success
> with reservation), zero or negative.
>
> - Hence the caller can be assured that "res" being positive can
> mean nobody failed with negative return, but it is not an
> unconditional success, which is signalled by value "res" being
> 0.
>
> I cannot quite tell which is the case, especially what is planned in
> the future. The proposed log message is a good place to explain the
> future course this code will take.
Okay, I will try to come up with a better commit message.
Actually, come to think of it, I will change the patch, as it is too
confusing. What I want is to preserve a positive return value in case of
merge conflicts, and that is exactly what I should do instead of playing
games with the Boolean OR operator.
Ciao,
Dscho
^ permalink raw reply
* Re: [musl] Re: Regression: git no longer works with musl libc's regex impl
From: James B @ 2016-10-05 11:59 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: musl, Rich Felker, Jeff King, git
In-Reply-To: <alpine.DEB.2.20.1610051231390.35196@virtualbox>
On Wed, 5 Oct 2016 12:41:50 +0200 (CEST)
Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> >
> > Wow, I don't know that Windows is a git's first-tier platform now,
>
> It is. Git for Windows is maintained by me, and I make as certain as I can
> that it works fine.
> And yes, we have download numbers to support my claim.
> The latest release is less than 24h old, but I can point you to Git for
> Windows 2.8.1 whose 32-bit installer was downloaded 397,273 times, and
> whose 64-bit installer was downloaded 3,780,079 times.
Number downloads does not make first-tier platform. You know that as well as everyone else.
First-tier support is the decision made by the maintainers that the entire features of the software must be available on those first tier platforms. So if Windows is indeed first-tier platform for git, it means any features that don't work on git version of Windows must not be used/developed or even castrated. That's a scary thought.
Being a maintainer for "Git for Windows" does not make one automatically as the maintainer for "git", although that can happen.
So this decision that "Windows is now a first-tier platform for git" - is your own opinion, or is this the collective opinion of *all* the git maintainers?
>
> > and Linux/POSIX second.
>
> This is not at all what I said, so please be careful of what you accuse
> me.
Yes, you did not say that. I said that. And I will say more. Git has Linux/POSIX roots. Attempting to "not use common POSIX features because they're not available on Windows" *does* make Linux/POSIX feels like second class platform for git. The way I see it, it should be *the other way around*.
It's a very sad day for a tool that was developed originally to maintain Linux kernel, by the Linux kernel author, now is restricted to avoid use/optimise on Linux/POSIX features *because* it has to run on another Windows ...
>
> What I said is that we never exploited the full POSIX standard, but that
> we made certain to use a subset of POSIX in Git which would be relatively
> easy to emulate using Windows' API.
All this just proves my point above.
And - I notice you use the pronoun "we" - is that a "royal we" (which means the entire point is your own or your cohorts position), or is it the official position of all git maintainers?
>
> > Are we talking about the same git that was originally written in Linus
> > Torvalds, and is used to manage Linux kernel?
>
> It was originally written by (not in) Linus Torvalds, and yes, the Linux
> kernel is one of its many users.
That was a rhetorical question.
>
> > Are you by any chance employed by Redmond, directly or indirectly?
>
> I am not exactly employed by Redmond, but by Microsoft (this is what you
> meant, I guess).
>
> I maintained Git for Windows in my spare time, next to a very demanding
> position in science, for eight years. In 2015, I joined Microsoft and part
> of my role is to maintain Git for Windows, allowing me to do a much better
> job at it.
Well thank you for being honest. I can see now why you responded the way you did (and still do). By being employed by Microsoft, and especially paid to work on Git for Windows, you have all the incentives to make it work best on Windows, and to make it as its first-tier platform within the limitation of Windows.
That in itself is not a problem - it only starts to become a problem when you try to cut down support for other platforms or stifle improvements on other platforms because "hey it makes it too hard to do those things in Windows".
>
> Of course, I do not only improve Git's Windows support, but contribute
> other patches, too. You might also appreciate the fact that some of my
> colleagues started contributing patches to Git that benefit all Git users.
>
By "colleagues" I assume other Microsoft employees?
I don't have a problem with that - thank you and your colleagues for making git better.
But that does not give the right to you to control that "if it doesn't work on Windows we shouldn't do it". If you do, and at the same time claim that musl-libc (which is Linux-only) support is unimportant compared to your 4 million Git-for-Windows downloads really don't do well for your or your employer's image. I don't have to say why - everyone outside Microsoft knows why.
In conclusion, I certainly hope that your view is not shared by the other git maintainers.
PS: Rich, sorry for the distraction. I have said what I want to say, so I'll bow out from this thread.
cheers,
James
^ permalink raw reply
* Re: [PATCH v2 04/25] sequencer: future-proof remove_sequencer_state()
From: Johannes Schindelin @ 2016-10-05 11:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <xmqqinu028tx.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 12 Sep 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > +static const char *get_dir(const struct replay_opts *opts)
> > +{
> > + return git_path_seq_dir();
> > +}
>
> Presumably this is what "In a couple of commits" meant, i.e. opts
> will be used soonish.
Exactly. The sequencer code was taught to use a state directory different
from rebase -i's (even if it quite clearly imitated rebase -i's approach),
to allow for rebase -i to run at the same time as the sequencer (by
calling it via cherry-pick).
So we need to be able to use different seq_dir locations, depending on the
mode we are running in.
I briefly considered consolidating them and using .git/rebase-merge/ as
state directory also for cherry-pick/revert, but that would cause
problems: I am surely not the only user who cherry-picks commits manually
while running interactive rebases.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v2 05/25] sequencer: allow the sequencer to take custody of malloc()ed data
From: Johannes Schindelin @ 2016-10-05 11:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <xmqqzinc295y.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 12 Sep 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > The sequencer is our attempt to lib-ify cherry-pick. Yet it behaves
> > like a one-shot command when it reads its configuration: memory is
> > allocated and released only when the command exits.
> >
> > This is kind of okay for git-cherry-pick, which *is* a one-shot
> > command. All the work to make the sequencer its work horse was
> > done to allow using the functionality as a library function, though,
> > including proper clean-up after use.
> >
> > This patch introduces an API to pass the responsibility of releasing
> > certain memory to the sequencer. Example:
> >
> > const char *label =
> > sequencer_entrust(opts, xstrfmt("From: %s", email));
>
> I thought we (not just me) were already pretty clear during the last
> round of review that we will not want this entrust() thing.
That does not match my understanding.
The problem is that we are building functionality for libgit.a, not merely
for a builtin that we know will simply exit() and take all allocated
memory with it.
The additional problem is that the sequencer was *already* meant for
libgit.a, yet simply strdup()s data left and right and assigns it to const
fields, purposefully wasting memory.
Sure, I can leave those memory leaks in, but then I also have to introduce
new ones via the rebase -i support.
If you prefer to accept such sloppy work, I will change it of course,
feeling dirty that it has my name on it.
Ciao,
Dscho
^ permalink raw reply
* Re: git commit -p with file arguments
From: Christian Neukirchen @ 2016-10-05 11:38 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Junio C Hamano
In-Reply-To: <20161005102633.GA9948@ash>
Duy Nguyen <pclouds@gmail.com> writes:
>> At the least I think we should clarify this in the document.
>
> How about something like this? Would it help?
I think it captures the current behavior well.
--
Christian Neukirchen <chneukirchen@gmail.com> http://chneukirchen.org
^ permalink raw reply
* Re: [PATCH v3 0/6] Pull out require_clean_work_tree() functionality from builtin/pull.c
From: Johannes Schindelin @ 2016-10-05 11:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqpongt034.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 4 Oct 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > This is the 5th last patch series of my work to accelerate interactive
> > rebases in particular on Windows.
>
> Offtopic, but I am always confused by what you might mean by this
> "nth last patch series". Is this series 5th from the last and we
> have four more to go?
Yes, we have four more to go:
- the patch series (called "prepare-sequencer" in my fork) preparing the
sequencer code for the next patch series, such as revamping the parser
for the edit script (or "todo script" as I used to say all the time, or
"insn sheet" as sequencer calls it),
- the patch series ("sequencer-i") teaching the sequencer to parse and
execute the commands of rebase -i's git-rebase-todo file,
- the patch series ("rebase--helper") introducing the builtin, and using
it from rebase -i, and finally
- the patch series ("rebase-i-extra") that moves more performance critical
bits and pieces from git-rebase--interactive.sh into the rebase--helper.
I had originally planned to stop at rebase--helper and invite other
developers to join the fun of making rebase -i a true builtin, but the
performance improvement was surprisingly disappointing before the
rebase--helper learned to skip unnecessary picks, to verify that the
script is valid, to expand/collapse the SHA-1s, and to rearrange
fixup!/squash! lines.
> In any case, after a quick re-read and comparison with the last
> round, I think this is in a good shape. I'd say that we would wait
> for a few days for others to comment and then merge it to 'next' if
> we missed nothing glaringly wrong.
Perfect!
Dscho
^ permalink raw reply
* Re: [PATCH v3 3/6] Make the require_clean_work_tree() function reusable
From: Johannes Schindelin @ 2016-10-05 11:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqy424t097.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 4 Oct 2016, Junio C Hamano wrote:
> I'd tweak the message while queuing, though.
>
> wt-status: make the require_clean_work_tree() function reusable
>
> The function "git pull" uses to stop the user when the working
> tree has changes is useful in other places.
I stumbled over this sentence. How about
The function used by "git pull" to stop [...]
instead?
Thanks,
Dscho
^ permalink raw reply
* Re: [musl] Re: Regression: git no longer works with musl libc's regex impl
From: Johannes Schindelin @ 2016-10-05 11:17 UTC (permalink / raw)
To: Rich Felker; +Cc: Jeff King, git, musl
In-Reply-To: <20161004173926.GA19318@brightrain.aerifal.cx>
Hi Rich,
On Tue, 4 Oct 2016, Rich Felker wrote:
> On Tue, Oct 04, 2016 at 06:08:33PM +0200, Johannes Schindelin wrote:
>
> > And lastly, the best alternative would be to teach musl about
> > REG_STARTEND, as it is rather useful a feature.
>
> Maybe, but it seems fundamentally costly to support -- it's extra
> state in the inner loops that imposes costly spill/reload on archs
> with too few registers (x86).
It is true that it could cause that.
I had a brief look at the source code (you use backtracking... hopefully
nobody uses musl to parse regular expressions from untrusted, or
inexperienced, sources [*1*]), and it seems that the regex code might
spill unnecessarily already (I see, for example, that the reg_notbol,
reg_noteol and reg_newline flags all use up complete int registers, not
merely bits of a single one).
It seems, specifically, that the *match_end_ofs parameter of the two
regexec backends is always set to point to eo, which is so far not
initialized. You could initialize it to -1 and set it to pmatch[0].rm_eo
if the REG_STARTEND flag is set. The GET_NEXT_WCHAR() macro would then
need to test something like
if (str_byte >= string + *match_end_ofs) {
ret = REG_NOMATCH; goto error_exit;
}
This does not handle non-zero pmatch[0].rm_so, though. I would probably
try to pass another input parameter for that, but I have not verified yet
that a "^" would be handled properly (if pmatch[0].rm_so > 0 and
REG_STARTEND is set, "^" should *not* match).
> I'll look at doing this when we overhaul/replace the regex
> implementation, and I'm happy to do some performance-regression tests
> for adding it now if someone has a simple patch (as was mentioned on the
> musl list).
I'd be interested to be kept in the loop, if you do not mind Cc:ing me.
Ciao,
Johannes
Footnote *1*:
http://stackstatus.net/post/147710624694/outage-postmortem-july-20-2016
^ permalink raw reply
* RE: [musl] Re: Regression: git no longer works with musl libc's regex impl
From: Johannes Schindelin @ 2016-10-05 10:49 UTC (permalink / raw)
To: writeonce; +Cc: musl, git, Jeff King
In-Reply-To: <20161004200057.dc30d64f61e5ec441c34ffd4f788e58e.efa66ead67.wbe@email15.godaddy.com>
Hi writeonce,
On Tue, 4 Oct 2016, writeonce@midipix.org wrote:
> < On Tue, 4 Oct 2016, Rich Felker wrote:
> <
> < > On Tue, Oct 04, 2016 at 11:27:22AM -0400, Jeff King wrote:
> < > > On Tue, Oct 04, 2016 at 11:08:48AM -0400, Rich Felker wrote:
> < > >
> < > > > 1. is nonzero mod page size, it just works; the remainder of the
> < > > > last page reads as zero bytes when mmapped.
> < > >
> < > > Is that a portable assumption?
> < >
> < > Yes.
> <
> < No, it is not. You quote POSIX, but the matter of the fact is that we
> < use a subset of POSIX in order to be able to keep things running on
> < Windows.
>
> As far as I can tell (and as the attached program may help demonstrate),
> the above assumption has been valid on all versions of Windows since at
> least Windows 2000.
And since W2K is already past its end of life, it would be safe for
practical considerations.
However, I have to add two comments to that:
- it is *not* guaranteed. The behavior is undefined, even if you see
consistent behavior so far. Future Windows versions might break that
assumption freely, though.
- some implementations of the REG_STARTEND feature have the nice property
that they can read past NUL characters. Granted, not all of them do
(AFAIU one example is FreeBSD itself, the first platform to sport
REG_STARTEND), but we at least reap the benefit whenever using a regex
that *can* read past NUL characters.
> In this context, one thing to remember is that the page-size for the mod
> operation is 4096, whereas the POSIX page-size (for the purpose of mmap
> and mremap) is 65536.
Indeed. A colleague of mine spotted the segfault when diffing a file that
was *exactly* 4,096 bytes.
> Note also that in the case of file-backed mapped sections, using
> kernel32.dll or msvcrt.dll or cygwin/newlib or midipix/musl is of little
> significance, specifically since all invoke ZwCreateSection and
> ZwMapViewOfSection under the hood.
Right. It's all backed by the very same kernel functions.
Ciao,
Johannes
^ permalink raw reply
* Re: "Purposes, Concepts,Misfits, and a Redesign of Git" (a research paper)
From: Duy Nguyen @ 2016-10-05 10:42 UTC (permalink / raw)
To: Jakub Narębski
Cc: Santiago Perez De Rosso, Konstantin Khomoutov, git,
Daniel Jackson, Greg Wilson
In-Reply-To: <CANQwDwcj15bk3uvjqnOwqqLFN_qOZCoWATssNBwD4kDTDfS6Hw@mail.gmail.com>
On Wed, Oct 5, 2016 at 5:14 PM, Jakub Narębski <jnareb@gmail.com> wrote:
> [git@vger.kernel.org does not accept HTML emails]
>
> I just hope that this email don't get mangled too much...
>
> On 5 October 2016 at 04:55, Santiago Perez De Rosso
> <sperezde@csail.mit.edu> wrote:
>> On Fri, Sep 30, 2016 at 6:25 PM Jakub Narębski <jnareb@gmail.com> wrote:
>>> W dniu 30.09.2016 o 18:14, Konstantin Khomoutov pisze:
>>>
>>>> The "It Will Never Work in Theory" blog has just posted a summary of a
>>>> study which tried to identify shortcomings in the design of Git.
>>>>
>>>> In the hope it might be interesting, I post this summary here.
>>>> URL: http://neverworkintheory.org/2016/09/30/rethinking-git.html
>>>
>>> I will comment on the article itself, not just on the summary.
>>>
>>> | 2.2 Git
>>> [...]
>>> | But tracked files cannot be ignored; to ignore a tracked file
>>> | one has to mark it as “assume unchanged.” This “assume
>>> | unchanged” file will not be recognized by add; to make it
>>> | tracked again this marking has to be removed.
>>>
>>> WRONG! Git has tracked files, untracked unignored files, and
>>> untracked ignored files (mostly considered unimportant).
>>>
>>> The "assume unchanged" bit is _performance_ optimization. It is not,
>>> and cannot be a 'ignore tracked files' bit - here lies lost work!!!
>>> You can use (imperfectly) "prefer worktree" bit hack instead.
>>>
>>> You can say, if 'ignoring change to tracked files' is motivation,
>>> or purpose, it lacks direct concept.
>>
>>
>> I don't see what's wrong with the paragraph you mention. I am aware of the
>> fact that assumed unchanged is intended to be used as a performance
>> optimization but that doesn't seem to be the way it is used in practice.
>> Users have appropriated the optimization and effectively turned into a
>> concept that serves the purpose of preventing the commit of a file. For
>> example:
>>
>> from http://gitready.com/intermediate/2009/02/18/temporarily-ignoring-files.html
>>
>> So, to temporarily ignore changes in a certain file, run:
>> git update-index --assume-unchanged <file>
>> ...
>>
>> from http://stackoverflow.com/questions/17195861/undo-git-update-index-assume-unchanged-file
>> The way you git ignore watching/tracking a particular dir/file.
>> you just run this:
>> git update-index --assume-unchanged <file>
>> ...
>>
>>
>> btw, this appropriation suggests that users want to be able to ignore
>> tracked files and they do what they can with what they are given (which
>> in this case means abusing the assumed unchanged bit).
>
> Yes, this is true that users may want to be able to ignore changes to
> tracked files (commit with dirty tree), but using `assume-unchanged` is
> wrong and dangerous solution. Unfortunately the advice to use it is
> surprisingly pervasive. I would thank you to not further this error.
> (Well, `skip-worktree` is newer, that's why it is lesser known, perhaps)
>
> To ignore tracked files you need to use `skip-worktree` bit.
>
> You can find the difference between `assume-unchanged` and
> `skip-worktree`, and when use which in:
> http://stackoverflow.com/questions/13630849/git-difference-between-assume-unchanged-and-skip-worktree
> http://fallengamer.livejournal.com/93321.html
> http://blog.stephan-partzsch.de/how-to-ignore-changes-in-tracked-files-with-git/
>
> The difference is that skip-worktree will not overwrite a file that is
> different from the version in the index, but assume-unchanged can. This
> means that the latter can OVERWRITE YOUR PRECIOUS CHANGES!
>
> Some people started to recommend it
> http://stackoverflow.com/questions/32251037/ignore-changes-to-a-tracked-file
> http://www.virtuouscode.com/2011/05/20/keep-local-modifications-in-git-tracked-files/
And since skip-worktree bits may be set/cleared freely when sparse
checkout mode is on, you should never manipulate these bits directly
if you also use sparse checkout.
>>> | *Detached Head* Suppose you are working on some branch
>>> | and realize that the last few commits you did are wrong, so
>>> | you decide to go back to an old commit to start over again.
>>> | You checkout that old commit and keep working creating
>>> | commits. You might be surprised to discover that these new
>>> | commits you’ve been working on belong to no branch at all.
>>> | To avoid losing them you need to create a new branch or reset
>>> | an existing one to point to the last commit.
>>>
>>> It would be hard to be surprised unless one is in habit of
>>> disregarding multi-line warning from Git... ;-)
>>
>> True if you are an expert user, but I can assure you novices will
>> find that situation baffling, even with the multi-line warnings.
Hmm... when you switch away from a detached HEAD, you are advised to
do "git branch <new-name> blah blah". How is it baffling? Genuine
question, maybe I have been using git for too long I just fail to see
it.
> True, the "detached HEAD" case (aka "unnamed branch") can be puzzling
> for Git users, and it has few uses (e.g. checking out the state of
> tag temporarily, to test it).
>
> I wonder if `git status` should be enhanced to tell user how to get
> out of "detached HEAD" situation -- it has lots of advices in it.
Detached HEAD is also present in interactive rebase or any command
that has --abort/--continue options. I don't think we need to tell the
user to get out of detached HEAD in that case. Just two cents if
someone is going to add this advice to git-status.
--
Duy
^ permalink raw reply
* Re: [musl] Re: Regression: git no longer works with musl libc's regex impl
From: Johannes Schindelin @ 2016-10-05 10:41 UTC (permalink / raw)
To: James B; +Cc: musl, Rich Felker, Jeff King, git
In-Reply-To: <20161005090625.683fdbbfac8164125dee6469@gmail.com>
Hi James,
On Wed, 5 Oct 2016, James B wrote:
> On Tue, 4 Oct 2016 18:08:33 +0200 (CEST)
> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>
> > No, it is not. You quote POSIX, but the matter of the fact is that we
> > use a subset of POSIX in order to be able to keep things running on
> > Windows.
> >
> > And quite honestly, there are lots of reasons to keep things running
> > on Windows, and even to favor Windows support over musl support. Over
> > four million reasons: the Git for Windows users.
>
> Wow, I don't know that Windows is a git's first-tier platform now,
It is. Git for Windows is maintained by me, and I make as certain as I can
that it works fine. And yes, we have download numbers to support my claim.
The latest release is less than 24h old, but I can point you to Git for
Windows 2.8.1 whose 32-bit installer was downloaded 397,273 times, and
whose 64-bit installer was downloaded 3,780,079 times.
> and Linux/POSIX second.
This is not at all what I said, so please be careful of what you accuse
me.
What I said is that we never exploited the full POSIX standard, but that
we made certain to use a subset of POSIX in Git which would be relatively
easy to emulate using Windows' API.
> Are we talking about the same git that was originally written in Linus
> Torvalds, and is used to manage Linux kernel?
It was originally written by (not in) Linus Torvalds, and yes, the Linux
kernel is one of its many users.
Git is not used only for the Linux kernel, though, and I am certain that
Linus agrees that it should not cater only to the Linux folks. Git is used
very widely in OSS as well as in the industry. So we, the Git developers,
kind of have an obligation to make things work in a much broader
perspective than you suggested.
> Are you by any chance employed by Redmond, directly or indirectly?
I am not exactly employed by Redmond, but by Microsoft (this is what you
meant, I guess).
I maintained Git for Windows in my spare time, next to a very demanding
position in science, for eight years. In 2015, I joined Microsoft and part
of my role is to maintain Git for Windows, allowing me to do a much better
job at it.
Of course, I do not only improve Git's Windows support, but contribute
other patches, too. You might also appreciate the fact that some of my
colleagues started contributing patches to Git that benefit all Git users.
> Sorry - can't help it.
I do not know why you are sorry, and I do not believe that you have to be.
Ciao,
Johannes
^ permalink raw reply
* Re: git commit -p with file arguments
From: Duy Nguyen @ 2016-10-05 10:26 UTC (permalink / raw)
To: Christian Neukirchen; +Cc: Git Mailing List, Junio C Hamano
In-Reply-To: <CACsJy8DOqoW8quz-6qSVR2+3aJau2V=qXCx_SoZvBpmU+9+Oxw@mail.gmail.com>
On Fri, Sep 09, 2016 at 05:54:30PM +0700, Duy Nguyen wrote:
> On Tue, Sep 6, 2016 at 4:08 AM, Christian Neukirchen
> <chneukirchen@gmail.com> wrote:
> > Hi,
> >
> > I noticed the following suprising behavior:
> >
> > % git --version
> > git version 2.10.0
> >
> > % git add bar
> > % git status -s
> > A bar
> > M foo
> >
> > % git commit -p foo
> > [stage a hunk]
> > ...
> > # Explicit paths specified without -i or -o; assuming --only paths...
> > # On branch master
> > # Changes to be committed:
> > # new file: bar
> > # modified: foo
> > #
> >
> > So why does it want to commit bar too, when I explicitly wanted to
> > commit foo only?
> >
> > This is not how "git commit files..." works, and the man page says
> >
> > 3.by listing files as arguments to the commit command, in which
> > case the commit will ignore changes staged in the index, and
> > instead record the current content of the listed files (which must
> > already be known to Git);
> >
> > I'd expect "git commit -p files..." to work like
> > "git add -p files... && git commit files...".
>
> ...
>
> At the least I think we should clarify this in the document.
How about something like this? Would it help?
-- 8< --
Subject: [PATCH] git-commit.txt: clarify --patch mode with pathspec
How pathspec is used, with and without --interactive/--patch, is
different. But this is not clear from the document. These changes hint
the user to keep reading (to option #5) instead of stopping at #2 and
assuming --patch/--interactive behaves the same way.
And since all the options listed here always mention how the index is
involved (or not) in the final commit, add that bit for #5 as well. This
"on top of the index" is implied when you head over git-add(1), but if
you just go straight to the "Interactive mode" and not read what git-add
is for, you may miss it.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/git-commit.txt | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index b0a294d..f2ab0ee 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -29,7 +29,8 @@ The content to be added can be specified in several ways:
2. by using 'git rm' to remove files from the working tree
and the index, again before using the 'commit' command;
-3. by listing files as arguments to the 'commit' command, in which
+3. by listing files as arguments to the 'commit' command
+ (without --interactive or --patch switch), in which
case the commit will ignore changes staged in the index, and instead
record the current content of the listed files (which must already
be known to Git);
@@ -41,7 +42,8 @@ The content to be added can be specified in several ways:
actual commit;
5. by using the --interactive or --patch switches with the 'commit' command
- to decide one by one which files or hunks should be part of the commit,
+ to decide one by one which files or hunks should be part of the commit
+ in addition to contents in the index,
before finalizing the operation. See the ``Interactive Mode'' section of
linkgit:git-add[1] to learn how to operate these modes.
--
2.8.2.524.g6ff3d78
-- 8< --
Duy
^ permalink raw reply related
* Re: Feature Request: user defined suffix for temp files created by git-mergetool
From: Josef Ridky @ 2016-10-05 10:19 UTC (permalink / raw)
To: David Aguilar; +Cc: Anatoly Borodin, git
In-Reply-To: <20161005094706.GA18574@gmail.com>
Hi David,
thank you very much for your reply.
Today I realized, that my attachment has been cut off, so I sent it in the morning [1].
I believe, that most of answer can be find in my previous email from this morning.
Only change, that should be done by this request is add possibility to edit hard-coded suffix of temporary files.
I don't think, that change hard-coded suffix to switch between two hard-coded suffix of temporary files is suitable solution, but as well it's not bad solution.
Please look at the patch [1] and tell me, what do you think about this change.
[1] http://marc.info/?l=git&m=147565363609649&w=2
Regards
Josef
| Sent: Wednesday, October 5, 2016 11:47:06 AM
|
| On Tue, Oct 04, 2016 at 01:18:47AM -0400, Josef Ridky wrote:
| > Hi Anatoly,
| >
| >
| > | Sent: Monday, October 3, 2016 5:18:44 PM
| > |
| > | Hi Josef,
| > |
| > |
| > | On Mon, Oct 3, 2016 at 8:36 AM, Josef Ridky <jridky@redhat.com> wrote:
| > | > In several projects, we are using git mergetool for comparing files
| > | > from
| > | > different folders.
| > | > Unfortunately, when we have opened three files for comparing using
| > | > meld
| > | > tool (e.q. Old_version -- Result -- New_version),
| > | > we can see only name of temporary files created by mergetool in the
| > | > labels
| > | > (e.g. foo_REMOTE -- foo_BASE -- foo_LOCAL)
| > | > and users (and sometime even we) are confused, which of the files
| > | > should
| > | > they edit and save.
| > |
| > | `git mergetool` just creates temporary files (with some temporary
| > | names) and calls `meld` (or `vimdiff`, etc) with the file names as
| > | parameters. So why wouldn't you call `meld` with the file names you
| > | want?
| >
| >
| > Because files, that we want, are temporary files created by
| > git mergetool and we are not able to change their name.
|
| [I didn't see your original patch, but we actually prefer inline
| patches in the email, as sent via `git send-email`.
| Documentation/SubmittingPatches has more details.
|
| Please also make sure to add a test to t/t7610-mergetool.sh
| exercising any new features.]
|
| Are you proposing support for config variables to control how
| the temporary files are named?
|
| e.g. something like "mergetool.strings.{local,remote,base}" for
| overriding the hard-coded {LOCAL,REMOTE,BASE} strings?
|
| I don't want to over-engineer it, but do you want to support
| executing a command to get the name, or is having a replacement
| sufficient?
|
| Now I'm curious... if replacing the strings is sufficient, what
| do you plan to call them? I can imagine maybe something like
| OURS, and THEIRS might be helpful since it matches the
| nomenclature already used by Git, e.g. "git merge -s ours".
|
| Since these are temporary files, changing these names might not
| be entirely out of the question. This might be a case where
| using the same words as a related Git feature might help reduce
| the mental burden of using mergetool. OURS and THEIRS are
| probably the only names that fit that category, IMO.
| BASE is already good enough (merge-base).
|
| The downside of making it configurable is that it can confuse
| users who use mergetool at someone else's desk where they've
| named these strings to "catty", "wombat", and "jimbo". This
| doesn't seem like the kind of place where we want to allow users
| to be creative, but we do care about having a good default.
|
| OURS and THEIRS are intuitive names, so switching existing users
| to those would not have much downside IMO, and it's a little
| less "I just merged a REMOTE branch" centric, which is good.
|
| Do you think these names should be changed?
| If so, did you have those names in mind, or something else
| entirely?
|
| cheers,
| --
| David
|
^ permalink raw reply
* Re: "Purposes, Concepts,Misfits, and a Redesign of Git" (a research paper)
From: Jakub Narębski @ 2016-10-05 10:14 UTC (permalink / raw)
To: Santiago Perez De Rosso
Cc: Konstantin Khomoutov, git, Daniel Jackson, Greg Wilson
In-Reply-To: <CAKbZu+BUOAjixTmEC4octseyJbMnFuaCTtLT9hx3H10=AECeKw@mail.gmail.com>
[git@vger.kernel.org does not accept HTML emails]
I just hope that this email don't get mangled too much...
On 5 October 2016 at 04:55, Santiago Perez De Rosso
<sperezde@csail.mit.edu> wrote:
> On Fri, Sep 30, 2016 at 6:25 PM Jakub Narębski <jnareb@gmail.com> wrote:
>> W dniu 30.09.2016 o 18:14, Konstantin Khomoutov pisze:
>>
>>> The "It Will Never Work in Theory" blog has just posted a summary of a
>>> study which tried to identify shortcomings in the design of Git.
>>>
>>> In the hope it might be interesting, I post this summary here.
>>> URL: http://neverworkintheory.org/2016/09/30/rethinking-git.html
>>
>> I will comment on the article itself, not just on the summary.
>>
>> | 2.2 Git
>> [...]
>> | But tracked files cannot be ignored; to ignore a tracked file
>> | one has to mark it as “assume unchanged.” This “assume
>> | unchanged” file will not be recognized by add; to make it
>> | tracked again this marking has to be removed.
>>
>> WRONG! Git has tracked files, untracked unignored files, and
>> untracked ignored files (mostly considered unimportant).
>>
>> The "assume unchanged" bit is _performance_ optimization. It is not,
>> and cannot be a 'ignore tracked files' bit - here lies lost work!!!
>> You can use (imperfectly) "prefer worktree" bit hack instead.
>>
>> You can say, if 'ignoring change to tracked files' is motivation,
>> or purpose, it lacks direct concept.
>
>
> I don't see what's wrong with the paragraph you mention. I am aware of the
> fact that assumed unchanged is intended to be used as a performance
> optimization but that doesn't seem to be the way it is used in practice.
> Users have appropriated the optimization and effectively turned into a
> concept that serves the purpose of preventing the commit of a file. For
> example:
>
> from http://gitready.com/intermediate/2009/02/18/temporarily-ignoring-files.html
>
> So, to temporarily ignore changes in a certain file, run:
> git update-index --assume-unchanged <file>
> ...
>
> from http://stackoverflow.com/questions/17195861/undo-git-update-index-assume-unchanged-file
> The way you git ignore watching/tracking a particular dir/file.
> you just run this:
> git update-index --assume-unchanged <file>
> ...
>
>
> btw, this appropriation suggests that users want to be able to ignore
> tracked files and they do what they can with what they are given (which
> in this case means abusing the assumed unchanged bit).
Yes, this is true that users may want to be able to ignore changes to
tracked files (commit with dirty tree), but using `assume-unchanged` is
wrong and dangerous solution. Unfortunately the advice to use it is
surprisingly pervasive. I would thank you to not further this error.
(Well, `skip-worktree` is newer, that's why it is lesser known, perhaps)
To ignore tracked files you need to use `skip-worktree` bit.
You can find the difference between `assume-unchanged` and
`skip-worktree`, and when use which in:
http://stackoverflow.com/questions/13630849/git-difference-between-assume-unchanged-and-skip-worktree
http://fallengamer.livejournal.com/93321.html
http://blog.stephan-partzsch.de/how-to-ignore-changes-in-tracked-files-with-git/
The difference is that skip-worktree will not overwrite a file that is
different from the version in the index, but assume-unchanged can. This
means that the latter can OVERWRITE YOUR PRECIOUS CHANGES!
Some people started to recommend it
http://stackoverflow.com/questions/32251037/ignore-changes-to-a-tracked-file
http://www.virtuouscode.com/2011/05/20/keep-local-modifications-in-git-tracked-files/
>> | The notion of a “remote branch” must not be confused
>> | with that of an “upstream branch.” An upstream branch is
>> | just a convenience for users: after the user assigns it to some
>> | branch, commands like pull and push default to use that
>> | branch for fetching and pushing changes if no branch is given
>> | as input.
>>
>> Actually "upstream branch" (and related "upstream repository")
>> are a concept, not only a convenience. They denote a branch
>> (usually in remote repository) which is intended to ultimately
>> include changes in given branch. Note that "upstream branch"
>> can be set separately for any given local branch.
>
>
> We never say upstream branch is not a concept. It even appears in the
> table.
Hmmm... I got onfused by words "is just a convenience", which for me
implies that it is not a concept.
>>
>>
>> One thing that can enormously help recovering from errors, and
>> is not covered in the list of concepts is REFLOG.
>
>
> Yes, the analysis is not exhaustive, there are other concepts missing
> too (e.g., "submodule")
I think reflog is something that every user should know about, and
useful for all. Submodules and subtrees is something situational,
needed only for a subset of users.
On the other hand the concept of "submodules" is something that it is
under active development, and it is assumed to be immature. So coming
up with a good (re)design for this feature, and/or good set of
concept, would be a very good thing.
Another thing that it is not present is the concept of "immutable
history", and how to deal with it (`git-notes`, `git-replace`), and
how it conflict with other concepts (interactive rebase and the
concept of "rewriting history"). But I agree with focusing on most
commonly known, used and encountered concepts and misfits.
>> [...]
>> | 3. Operational Misfits
>> [...]
>> | *Saving Changes* Suppose you are in the middle of a long
>> | task and want to save your changes, so that they can be later
>> | retrieved in case of failure. How would you do that?
>>
>> You would use `git stash` or `git stash --include-untracked`!
>> In more complicated situations (during long-running operation
>> like resolving merge conflicts, interactive rebase, or finding
>> bugs with bisect) with modern Git you can create a new separate
>> working area with `git worktree`.
>
> A general comment regarding misfits: misfits correspond to scenarios
> in which Git behaves in a way that is unpredictable or inconvenient.
> It doesn't mean that a task has to be impossible to do in order for
> something to be a misfit.
If there were one-command solution, and the problem was that users
don't know about it, I would say that it is not a misfit. But,
admittedly, it is not the case here...
>> | *Detached Head* Suppose you are working on some branch
>> | and realize that the last few commits you did are wrong, so
>> | you decide to go back to an old commit to start over again.
>> | You checkout that old commit and keep working creating
>> | commits. You might be surprised to discover that these new
>> | commits you’ve been working on belong to no branch at all.
>> | To avoid losing them you need to create a new branch or reset
>> | an existing one to point to the last commit.
>>
>> It would be hard to be surprised unless one is in habit of
>> disregarding multi-line warning from Git... ;-)
>
> True if you are an expert user, but I can assure you novices will
> find that situation baffling, even with the multi-line warnings.
True, the "detached HEAD" case (aka "unnamed branch") can be puzzling
for Git users, and it has few uses (e.g. checking out the state of
tag temporarily, to test it).
I wonder if `git status` should be enhanced to tell user how to get
out of "detached HEAD" situation -- it has lots of advices in it.
>> [...]
>> | The problem
>> | is the lack of connection between this purpose and the highlevel
>> | purposes for version control, which suggests that the
>> | introduction of stashing might be to patch flaws in the design
>> | of Git and not to satisfy a requirement of version control.
>>
>> Or the problem might be that you are missing some (maybe minor)
>> requirement of version control system. Just saying...
>
> What would that purpose be? and why would you say that's a
> high-level purpose for version control and not one that's
> git-specific?
The stash (or rather its equivalent) is not something Git specific.
It is present also in other version control systems, among others:
* Mercurial: as 'shelve' extension (in core since 1.8)
* Bazaar: as 'bzr shelve' command
* Fossil: as 'fossil stash' command (with subcommands)
* Subversion: Shelve planned for 1.10 (2017?)
I would say that 'stash' could be considered about isolating work on
different features, different sub-branch sized parallel work.
But it might be that stash doesn't have connection with highlevel
purposes for version control, and that it is purely convenience
feature. Just playing the role of Advocatus Diaboli (important in
scientific works, isn' it?)...
>> [...]
>> | 7. Gitless
>> |
>> | 7.1 Overview
>> |
>> | Gitless has no staging area, and the only file classifications
>> | are “tracked,” “untracked,” “ignored,” and “in conflict.”
>>
>> Without staging area, I wonder how you would be able to handle
>> well different types of integration conflicts, which are not
>> limited to CONFLICT(content).
>>
>> You also loose the ability to select subset of *changes* to
>> be committed, not only files. This is often very useful, see
>> http://tomayko.com/writings/the-thing-about-git
>> http://2ndscale.com/rtomayko/2008/the-thing-about-git
>
> This is described later:
Right, my mistake, I have missed this. I should have read article in
full, then respond, rather than reply as I go...
> Common use cases for the staging area in Git are to select files to commit,
> split up a large change into multiple commits, and review the changes
> selected to be committed. We address the first by providing a more flexible
> commit command that lets the user easily customize the set of files to
> commit (with only, include and exclude flags). For the second use case we
> have a partial flag in commit that allows the user to interactively select
> segments of files to commit (like Git’s commit --patch). Finally, our diff
> command accepts the same only, include and exclude flags to customize the
> set of files to be diffed. There could be other use cases for the staging
> area that Gitless doesn’t handle well but we expect these to be fairly
> infrequent.
I wrote later that Git offers more flexibility with respect of
interactive edition of the staging area (additive, substractive, from
HEAD, from arbitrary commit) and checking the prepared state (add <->
diff <-> stash --keep-index + test + unstash)... but I think this is a
matter of implementing interface for those fetures to 'partial'
commit. It might be less flexible, and less powerfull, but it would
be there.
> Note that Gitless is built on top of Git so we do have a staging area, the
> difference is that unlike Git, in Gitless the index is hidden from the user.
All right.
I have just an idea of using the index (or rather its extensions) to
implement sub-commit history (forgotten after committing), as a way to
resolve misfit about "commit" concept and (P1) vs (P2) purposes...
>> [...]
>> | Also, there
>> | is no possible way of getting in a “detached head” state; at
>> | any time, the user is always working on some branch (the
>> | “current” branch). Head is a per-branch reference to the last
>> | commit of the branch.
>>
>> How do you solve the problem of checking out the state of
>> the tag, that is the state of repository at given revision?
>
> You can't checkout a tag, you would have to create a new branch with
> its head equal to the tag, and switch to that branch.
For the purpose of testing a state at a tag (for example as a
prerequisite to bisection), it fills like unnecessarily complicated
solution, a new misfit. But I guess that you consider "detached HEAD"
misfit to be more important to get rid of.
>> Also during some long lived multi-step operations, like bisect
>> or interactive rebase, you are not really on any branch,
>
> In Gitless we don't have bisect but for rebase (fuse in Gitless) we
> record the current branch.
No bisect? This is very useful feature. Though it might be done
without detached HEAD, but with specialized pseudo-branch 'bisect' (as
it was done in earlier versions of Git, or maybe even now).
Anyway, for [interactive] rebase / transplant / graft / fuse you need
to be able to abort an operation and return to the state before
staring rebase. Though you can or do solve this by remembering
the starting position.
>> | 7.2.1 Discussion
>> [...]
>> | There could be other use cases for the
>> | staging area that Gitless doesn’t handle well but we expect
>> | these to be fairly infrequent.
>>
>> Like handling merge conflict...??? Infrequent doesn't mean
>> unimportant.
>
> I regard handling merge conflicts as very important. Why do you need
> an explicit staging area for this? Note that Gitless has a staging
> area, it's just hidden from the user. The `gl diff` or `gl status`
> command should be able to show the same conflict information.
Well, the staging area was created (also?) to handle merges. Though
perhaps it doesn't need to be explicit to be useful...
Best regards,
--
Jakub Narębski
^ permalink raw reply
* Re: Feature Request: user defined suffix for temp files created by git-mergetool
From: David Aguilar @ 2016-10-05 9:47 UTC (permalink / raw)
To: Josef Ridky; +Cc: Anatoly Borodin, git
In-Reply-To: <597741871.671633.1475558327029.JavaMail.zimbra@redhat.com>
On Tue, Oct 04, 2016 at 01:18:47AM -0400, Josef Ridky wrote:
> Hi Anatoly,
>
>
> | Sent: Monday, October 3, 2016 5:18:44 PM
> |
> | Hi Josef,
> |
> |
> | On Mon, Oct 3, 2016 at 8:36 AM, Josef Ridky <jridky@redhat.com> wrote:
> | > In several projects, we are using git mergetool for comparing files from
> | > different folders.
> | > Unfortunately, when we have opened three files for comparing using meld
> | > tool (e.q. Old_version -- Result -- New_version),
> | > we can see only name of temporary files created by mergetool in the labels
> | > (e.g. foo_REMOTE -- foo_BASE -- foo_LOCAL)
> | > and users (and sometime even we) are confused, which of the files should
> | > they edit and save.
> |
> | `git mergetool` just creates temporary files (with some temporary
> | names) and calls `meld` (or `vimdiff`, etc) with the file names as
> | parameters. So why wouldn't you call `meld` with the file names you
> | want?
>
>
> Because files, that we want, are temporary files created by
> git mergetool and we are not able to change their name.
[I didn't see your original patch, but we actually prefer inline
patches in the email, as sent via `git send-email`.
Documentation/SubmittingPatches has more details.
Please also make sure to add a test to t/t7610-mergetool.sh
exercising any new features.]
Are you proposing support for config variables to control how
the temporary files are named?
e.g. something like "mergetool.strings.{local,remote,base}" for
overriding the hard-coded {LOCAL,REMOTE,BASE} strings?
I don't want to over-engineer it, but do you want to support
executing a command to get the name, or is having a replacement
sufficient?
Now I'm curious... if replacing the strings is sufficient, what
do you plan to call them? I can imagine maybe something like
OURS, and THEIRS might be helpful since it matches the
nomenclature already used by Git, e.g. "git merge -s ours".
Since these are temporary files, changing these names might not
be entirely out of the question. This might be a case where
using the same words as a related Git feature might help reduce
the mental burden of using mergetool. OURS and THEIRS are
probably the only names that fit that category, IMO.
BASE is already good enough (merge-base).
The downside of making it configurable is that it can confuse
users who use mergetool at someone else's desk where they've
named these strings to "catty", "wombat", and "jimbo". This
doesn't seem like the kind of place where we want to allow users
to be creative, but we do care about having a good default.
OURS and THEIRS are intuitive names, so switching existing users
to those would not have much downside IMO, and it's a little
less "I just merged a REMOTE branch" centric, which is good.
Do you think these names should be changed?
If so, did you have those names in mind, or something else
entirely?
cheers,
--
David
^ permalink raw reply
* Re: [PATCH 1/3] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"
From: Duy Nguyen @ 2016-10-05 9:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <xmqqd1jgw0nx.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 4, 2016 at 11:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> We don't use it internally _yet_. I need to go through all the
>> external diff code and see --shift-ita should be there. The end goal
>> is still changing the default behavior and getting rid of --shift-ita,
>
> I do not agree with that endgame, and quite honestly I do not want
> to waste time reviewing such a series.
I do not believe current "diff" behavior wrt. i-t-a entries is right
either. There's no point in pursuing this series then. Feel free to
revert 3f6d56d (commit: ignore intent-to-add entries instead of
refusing - 2012-02-07) and bring back the old i-t-a behavior. All the
problems would go away.
--
Duy
^ permalink raw reply
* Re: Feature Request: user defined suffix for temp files created by git-mergetool
From: Josef Ridky @ 2016-10-05 7:47 UTC (permalink / raw)
To: git
In-Reply-To: <1329039097.128066.1475476591437.JavaMail.zimbra@redhat.com>
Hi,
I have just realize, that my attachment has been cut off from my previous message.
Below you can find patch with requested change.
Add support for user defined suffix part of name of temporary files
created by git mergetool
---
Documentation/git-mergetool.txt | 26 ++++++++++++++++++++-
git-mergetool.sh | 52 +++++++++++++++++++++++++++++++++++++----
2 files changed, 72 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index e846c2e..6cf5935 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -8,7 +8,7 @@ git-mergetool - Run merge conflict resolution tools to resolve merge conflicts
SYNOPSIS
--------
[verse]
-'git mergetool' [--tool=<tool>] [-y | --[no-]prompt] [<file>...]
+'git mergetool' [--tool=<tool>] [-y | --[no-]prompt] [--local=<name>] [--remote=<name>] [--backup=<name>] [--base=<name>] [<file>...]
DESCRIPTION
-----------
@@ -79,6 +79,30 @@ success of the resolution after the custom tool has exited.
Prompt before each invocation of the merge resolution program
to give the user a chance to skip the path.
+--local=<name>::
+ Use string from <name> as part of suffix of name of temporary
+ file (local) for merging. If not used or is equal with any
+ other (remote|backup|base), default value is used.
+ Default suffix is LOCAL.
+
+--remote=<name>::
+ Use string from <name> as part of suffix of name of temporary
+ file (remote) for merging. If not used or is equal with any
+ other (local|backup|base), default value is used.
+ Default suffix is REMOTE.
+
+--backup=<name>::
+ Use string from <name> as part of suffix of name of temporary
+ file (backup) for merging. If not used or is equal with any
+ other (local|remote|base), default value is used.
+ Default suffix is BACKUP.
+
+--base=<name>::
+ Use string from <name> as part of suffix of name of temporary
+ file (base) for merging. If not used or is equal with any
+ other (local|remote|backup), default value is used.
+ Default suffix is BASE.
+
TEMPORARY FILES
---------------
`git mergetool` creates `*.orig` backup files while resolving merges.
diff --git a/git-mergetool.sh b/git-mergetool.sh
index bf86270..e3433ee 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -8,11 +8,18 @@
# at the discretion of Junio C Hamano.
#
-USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] ...'
+USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [--local=name] [--remote=name] [--backup=name] [--base=name] [file to merge] ...'
SUBDIRECTORY_OK=Yes
NONGIT_OK=Yes
OPTIONS_SPEC=
TOOL_MODE=merge
+
+#optional name space convention
+local_name=""
+remote_name=""
+base_name=""
+backup_name=""
+
. git-sh-setup
. git-mergetool--lib
@@ -271,10 +278,33 @@ merge_file () {
BASE=${BASE##*/}
fi
- BACKUP="$MERGETOOL_TMPDIR/${BASE}_BACKUP_$$$ext"
- LOCAL="$MERGETOOL_TMPDIR/${BASE}_LOCAL_$$$ext"
- REMOTE="$MERGETOOL_TMPDIR/${BASE}_REMOTE_$$$ext"
- BASE="$MERGETOOL_TMPDIR/${BASE}_BASE_$$$ext"
+ if [ "$local_name" != "" ] && [ "$local_name" != "$remote_name" ] && [ "$local_name" != "$backup_name" ] && [ "$local_name" != "$base_name" ]
+ then
+ LOCAL="$MERGETOOL_TMPDIR/${BASE}_${local_name}_$$$ext"
+ else
+ LOCAL="$MERGETOOL_TMPDIR/${BASE}_LOCAL_$$$ext"
+ fi
+
+ if [ "$remote_name" != "" ] && [ "$remote_name" != "$local_name" ] && [ "$remote_name" != "$backup_name" ] && [ "$remote_name" != "$base_name" ]
+ then
+ REMOTE="$MERGETOOL_TMPDIR/${BASE}_${remote_name}_$$$ext"
+ else
+ REMOTE="$MERGETOOL_TMPDIR/${BASE}_REMOTE_$$$ext"
+ fi
+
+ if [ "$backup_name" != "" ] && [ "$backup_name" != "$local_name" ] && [ "$backup_name" != "$remote_name" ] && [ "$backup_name" != "$base_name" ]
+ then
+ BACKUP="$MERGETOOL_TMPDIR/${BASE}_${backup_name}_$$$ext"
+ else
+ BACKUP="$MERGETOOL_TMPDIR/${BASE}_BACKUP_$$$ext"
+ fi
+
+ if [ "$base_name" != "" ] && [ "$base_name" != "$local_name" ] && [ "$base_name" != "$remote_name" ] && [ "$base_name" != "$backup_name" ]
+ then
+ BASE="$MERGETOOL_TMPDIR/${BASE}_${base_name}_$$$ext"
+ else
+ BASE="$MERGETOOL_TMPDIR/${BASE}_BASE_$$$ext"
+ fi
base_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==1) print $1;}')
local_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==2) print $1;}')
@@ -396,6 +426,18 @@ do
--prompt)
prompt=true
;;
+ --local=*)
+ local_name=${1#--local=}
+ ;;
+ --remote=*)
+ remote_name=${1#--remote=}
+ ;;
+ --base=*)
+ base_name=${1#--base=}
+ ;;
+ --backup=*)
+ backup_name=${1#--backup=}
+ ;;
--)
shift
break
--
2.7.4
^ permalink raw reply related
* Re: [PATCH 18/18] alternates: use fspathcmp to detect duplicates
From: Jeff King @ 2016-10-05 3:54 UTC (permalink / raw)
To: git, René Scharfe
In-Reply-To: <20161005023455.GA6215@pug.qqx.org>
On Tue, Oct 04, 2016 at 10:34:55PM -0400, Aaron Schrab wrote:
> At 16:36 -0400 03 Oct 2016, Jeff King <peff@peff.net> wrote:
> > On a case-insensitive filesystem, we should realize that
> > "a/objects" and "A/objects" are the same path.
>
> The current repository being on a case-insensitive filesystem doesn't
> guarantee that the alternates are as well.
>
> On the other hand, I suspect that people who use a case-insensitive
> filesystem would be less likely to use names which differ only by case.
True. I don't think we actually have enough information to make the
correct comparison (not only that, but I think that fspathcmp() can
sometimes be fooled by a path which is only partially case-insensitive
due to a case-insensitive filesystem mounted on a case-sensitive one).
Still, I think in practice this is likely to do more good than harm, as
I'd guess that being on a single filesystem is the common case.
-Peff
^ permalink raw reply
* RE: [musl] Re: Regression: git no longer works with musl libc's regex impl
From: writeonce @ 2016-10-05 3:00 UTC (permalink / raw)
To: musl; +Cc: Johannes.Schindelin, git, Jeff King
[-- Attachment #1: Type: text/plain, Size: 2834 bytes --]
<
<
< -------- Original Message --------
< Subject: [musl] Re: Regression: git no longer works with musl libc's
< regex impl
< From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
< Date: Tue, October 04, 2016 9:08 am
< To: Rich Felker <dalias@libc.org>
< Cc: Jeff King <peff@peff.net>, git@vger.kernel.org,
< musl@lists.openwall.com
<
< Hi Rich,
<
< On Tue, 4 Oct 2016, Rich Felker wrote:
<
< > On Tue, Oct 04, 2016 at 11:27:22AM -0400, Jeff King wrote:
< > > On Tue, Oct 04, 2016 at 11:08:48AM -0400, Rich Felker wrote:
< > >
< > > > 1. is nonzero mod page size, it just works; the remainder of the
last
< > > > page reads as zero bytes when mmapped.
< > >
< > > Is that a portable assumption?
< >
< > Yes.
<
< No, it is not. You quote POSIX, but the matter of the fact is that we
use
< a subset of POSIX in order to be able to keep things running on
Windows.
<
As far as I can tell (and as the attached program may help demonstrate),
the above assumption has been valid on all versions of Windows since at
least Windows 2000. In this context, one thing to remember is that the
page-size for the mod operation is 4096, whereas the POSIX page-size
(for the purpose of mmap and mremap) is 65536. Note also that in the
case of file-backed mapped sections, using kernel32.dll or msvcrt.dll or
cygwin/newlib or midipix/musl is of little significance, specifically
since all invoke ZwCreateSection and ZwMapViewOfSection under the hood.
HTH,
midipix
< And quite honestly, there are lots of reasons to keep things running
on
< Windows, and even to favor Windows support over musl support. Over
four
< million reasons: the Git for Windows users.
<
< So rather than getting into an ideological discussion about "broken"
< systems, it would be good to keep things practical, realizing that
those
< users make up a very real chunk of all of Git's users.
<
< As to making NO_REGEX conditional on REG_STARTEND: you are talking
about
< apples and oranges here. NO_REGEX is a Makefile flag, while
REG_STARTEND
< is a C preprocessor macro.
<
< Unless you can convince the rest of the Git developers (you would not
< convince me) to simulate autoconf by compiling an executable every
time
< `make` is run, to determine whether REG_STARTEND is defined, this is a
< no-go.
<
< However, you *can* use autoconf directly, and come up with a patch to
our
< configure.ac that detects the absence of REG_STARTEND and sets
NO_REGEX=1.
<
< Alternatively, you can set NO_REGEX=1 in your config.mak.
<
< Or, if you use one of the auto-detected cases in config.mak.uname, you
< could patch it to set NO_REGEX=1.
<
< And lastly, the best alternative would be to teach musl about
< REG_STARTEND, as it is rather useful a feature.
<
< Ciao,
< Johannes
<
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: mmap_file.c --]
[-- Type: text/x-c; name="mmap_file.c";, Size: 1487 bytes --]
#define _XOPEN_SOURCE 500
#define NT_FILE_BACKED_PAGE_SIZE 4096
#include <unistd.h>
#include <fcntl.h>
#include <stdio.h>
#include <errno.h>
#include <sys/mman.h>
static int test_file_backed_mmap(int size)
{
int fd;
int exsize;
void * addr;
char * test;
char * cap;
char name[12];
/* create test file of desired size */
sprintf(name,"%d.tmp",size);
if ((fd = creat(name,0755)) < 0)
return -1;
if (ftruncate(fd,size))
return -1;
close(fd);
/* write 'W' to all bytes of test file */
if ((fd = open(name,O_RDWR,0)) < 0)
return -1;
if ((addr = mmap(0,size,PROT_WRITE,MAP_SHARED,fd,0)) == MAP_FAILED)
return -1;
close(fd);
test = (char *)addr;
cap = (char *)addr + size;
for (; test<cap; test++)
*test = 'W';
munmap(addr,size);
/* map file for read access */
if ((fd = open(name,O_RDONLY,0)) < 0)
return -1;
if ((addr = mmap(0,size,PROT_READ,MAP_PRIVATE,fd,0)) == MAP_FAILED)
return -1;
close(fd);
/* test */
exsize = size + NT_FILE_BACKED_PAGE_SIZE;
exsize &= ~(NT_FILE_BACKED_PAGE_SIZE - 1);
test = (char *)addr + size;
cap = (char *)addr + exsize;
for (; test<cap; test++)
if (*test)
return -1;
/* clean-up */
munmap(addr,size);
unlink(name);
return 0;
}
int main(void)
{
int i;
for (i=1; i<65536; i++)
if (i % NT_FILE_BACKED_PAGE_SIZE)
if (test_file_backed_mmap(i))
return 2;
return 0;
}
^ permalink raw reply
* Re: [PATCH 18/18] alternates: use fspathcmp to detect duplicates
From: Aaron Schrab @ 2016-10-05 2:34 UTC (permalink / raw)
To: Jeff King; +Cc: git, René Scharfe
In-Reply-To: <20161003203626.styj2vwcmgwnpx4v@sigill.intra.peff.net>
At 16:36 -0400 03 Oct 2016, Jeff King <peff@peff.net> wrote:
>On a case-insensitive filesystem, we should realize that
>"a/objects" and "A/objects" are the same path.
The current repository being on a case-insensitive filesystem doesn't
guarantee that the alternates are as well.
On the other hand, I suspect that people who use a case-insensitive
filesystem would be less likely to use names which differ only by case.
^ permalink raw reply
* Re: [PATCH 5/5] versioncmp: cope with common leading parts in versionsort.prereleaseSuffix
From: SZEDER Gábor @ 2016-10-05 1:33 UTC (permalink / raw)
To: Jeff King, Junio C Hamano
Cc: Leho Kraav, Nguyễn Thái Ngọc Duy, git
In-Reply-To: <20160907174841.Horde.Ru1LBEeLKomznlWVG-ZnS-Q@webmail.informatik.kit.edu>
Quoting SZEDER Gábor <szeder@ira.uka.de>:
> Quoting SZEDER Gábor <szeder@ira.uka.de>:
>
>> Version sort with prerelease reordering sometimes puts tagnames in the
>> wrong order, when the common part of two compared tagnames ends with
>> the leading character(s) of one or more configured prerelease
>> suffixes.
>>
>> $ git config --get-all versionsort.prereleaseSuffix
>> -beta
>> $ git tag -l --sort=version:refname v2.1.*
>> v2.1.0-beta-2
>> v2.1.0-beta-3
>> v2.1.0
>> v2.1.0-RC1
>> v2.1.0-RC2
>> v2.1.0-beta-1
>> v2.1.1
>> v2.1.2
>>
>> The reason is that when comparing a pair of tagnames, first
>> versioncmp() looks for the first different character in a pair of
>> tagnames, and then the swap_prereleases() helper function checks for
>> prerelease suffixes _starting at_ that character. Thus, when in the
>> above example the sorting algorithm happens to compare the tagnames
>> "v2.1.0-beta-1" and "v2.1.0-RC2", swap_prereleases() will try to match
>> the suffix "-beta" against "beta-1" to no avail, and the two tagnames
>> erroneously end up being ordered lexicographically.
>>
>> To fix this issue change swap_prereleases() to look for configured
>> prerelease suffixes containing that first different character.
>
> Now, while I believe this is the right thing to do to fix this bug,
> there is a corner case, where multiple configured prerelease suffixes
> might match the same tagname:
>
> $ git config --get-all versionsort.prereleaseSuffix
> -bar
> -baz
> -foo-bar
> $ ~/src/git/git tag -l --sort=version:refname
> v1.0-foo-bar
> v1.0-foo-baz
>
> I.e. when comparing these two tags, both "-bar" and "-foo-bar" would
> match "v1.0-foo-bar", and as "-bar" comes first in the config file,
> it wins, and "v1.0-foo-bar" is ordered first. An argument could be
> made to prefer longer matches, in which case "v1.0-foo-bar" would be
> ordered according to "-foo-bar", i.e. as second. However, I don't
> know what that argument could be, to me neither behavior is better
> than the other, but the implementation of the "longest match counts"
> would certainly be more complicated.
>
> The argument I would make is that this is a pathological corner case
> that doesn't worth worrying about.
After having slept on this a couple of times, I think the longest
matching prerelease suffix should determine the sorting order.
A release tag usually consists of an optional prefix, e.g. 'v' or
'snapshot-', followed by the actual version number, followed by an
optional suffix. In the contrived example quoted above this suffix
is '-foo-bar', thus it feels wrong to match '-bar' against it, when
the user explicitly configured '-foo-bar' as prerelease suffix as
well.
Then here is a more realistic case for sorting based on the longest
matching suffix, where
- a longer suffix starts with the shorter one,
- and the longer suffix comes after the shorter one in the
configuration.
With my patches it looks like this:
$ git -c versionsort.prereleasesuffix=-pre \
-c versionsort.prereleasesuffix=-prerelease \
tag -l --sort=version:refname
v1.0.0-prerelease1
v1.0.0-pre1
v1.0.0-pre2
v1.0.0
Yeah, having both '-pre' and '-prerelease' suffixes seems somewhat
silly, but who knows what similar but more reasonable prerelease
suffixes e.g. non-English speaking users might use in their native
language.
Anyway, my intuition says that any '-prereleaseX' tags should come
after all the '-preX' tags. (Ironically, current git just happens
to get this particular case right.)
My patches get this wrong, because they look for prerelease suffixes
_containing_ the first different character. However, when a '-preX'
and a '-prereleaseX' tag are compared, then the whole '-pre' suffix
is part of the common part, thus it doesn't match, only '-prerelease'
matches.
So, to sort this case right the implementation should
- look for a prerelease suffix containing the first different
character or ending right before the first different character,
(This means that when comparing 'v1.0.0-pre1' and 'v1.0.0-pre2'
swap_prereleases() would match '-pre' in both: no big deal, it
should return "undecided" and let the caller do the right thing
by sorting based on '1' and '2')
- and sort a tag based on the longest matching prerelease suffix.
(In my quoted email above I alluded that its implementation must
be more complicated. No, it turns out that it actually isn't.)
(Just for the record: it's still not 100% foolproof, though. Someone
asking for trouble might use letters instead of numbers to indicate
subsequent prereleases, and might have tags 'v1.0-prea', 'v1.0-preb',
..., 'v1.0-prer', and 'v1.0-prerelease'. In this case the sorting
algorithm might happen to decide to compare 'v1.0-prer' and
'v1.0-prerelease', and since the common part is 'v1.0-prer' it will
fail to recognize the '-pre' suffix. I don't see how this case could
be supported in a sane way, or why it's worth to support it.)
And a final sidenote: sorting based on the longest matching suffix
also allows us to (ab)use version sort with prerelease suffixes to
sort postrelease tags as we please, too:
$ ~/src/git/git -c versionsort.prereleasesuffix=-alpha \
-c versionsort.prereleasesuffix=-beta \
-c versionsort.prereleasesuffix= \
-c versionsort.prereleasesuffix=-gamma \
-c versionsort.prereleasesuffix=-delta \
tag -l --sort=version:refname 'v3.0*'
v3.0-alpha1
v3.0-beta1
v3.0
v3.0-gamma1
v3.0-delta1
So, unless I hear a counterargument, I will submit a reroll with
longest matching suffix determining sort order added on top once I
get around to finish up its commit message.
Best,
Gábor
^ permalink raw reply
* Re: [RFC/PATCH] attr: Document a new possible thread safe API
From: Stefan Beller @ 2016-10-04 23:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Brandon Williams
In-Reply-To: <xmqqtwcrr9l6.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 4, 2016 at 4:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> diff --git a/Documentation/technical/api-gitattributes.txt b/Documentation/technical/api-gitattributes.txt
>> index 92fc32a..940617e 100644
>> --- a/Documentation/technical/api-gitattributes.txt
>> +++ b/Documentation/technical/api-gitattributes.txt
>> @@ -59,7 +59,10 @@ Querying Specific Attributes
>> empty `struct git_attr_check` can be prepared by calling
>> `git_attr_check_alloc()` function and then attributes you want to
>> ask about can be added to it with `git_attr_check_append()`
>> - function.
>> + function. git_attr_check_initl is thread safe, i.e. you can call it
>> + from different threads at the same time; internally however only one
>> + call at a time is processed. If the calls from different threads have
>> + the same arguments, the returned `git_attr_check` may be the same.
>
> I do not think this is enough. Look at the example for _initl() and
> notice that it keeps the "singleton static check that is initialized
> by the very first caller if the caller notices it is NULL" pattern.
>
> One way to hide that may be to pass the address of that singleton
> pointer to _initl(), so that it can do the "has it been initialized?
> If not, let's prepare the thing" under lock.
Oh, I see. Yeah that makes sense.
>
>> @@ -89,15 +92,21 @@ static void setup_check(void)
>>
>> ------------
>> const char *path;
>> + struct git_attr_check *result;
>>
>> setup_check();
>> - git_check_attr(path, check);
>> + result = git_check_attr(path, check);
>
> I haven't formed a firm opinion, but I suspect your thinking might
> be clouded by too much staring at the current implementation that
> has <attr>,<value> pairs inside git_attr_check. Traditionally, the
> attr subsystem used the same type for the query question and the
> query answer the same type, but it does not have to stay to be the
> case at all. Have you considered that we are allowed to make these
> two types distinct?
I thought about that, but as I concluded that the get_all_attrs doesn't need
conversion to a threading environment, we can keep it as is.
When keeping the get_all_attrs as is, we need to keep the data structures
as is, (i.e. key,value pair inside git_check_attr), so introducing a new
data type seemed not useful for the threaded part.
> A caller can share the same question instance
> (i.e. the set of interned <attr>, in git_attr_check) with other
> threads as that is a read-only thing, but each of the callers would
> want to have the result array on its own stack if possible
> (e.g. when asking for a known set of attributes, which is the
> majority of the case) to avoid allocation cost. I'd expect the most
> typical caller to be
>
> static struct git_attr_check *check;
> struct git_attr_result result[2]; /* we want two */
>
> git_attr_check_initl(&check, "crlf", "ident", NULL);
> git_check_attr(path, check, result);
> /* result[0] has "crlf", result[1] has "ident" */
>
> or something like that.
I see, that seems to be a clean API. So git_attr_check_initl
will lock the mutex and once it got the mutex it can either
* return early as someone else did the work
* needs to do the actual work
and then unlock. In any case the work was done.
git_check_attr, which runs in all threads points to the same check,
but gets the different results.
Ok, I'll go in that direction then.
Thanks,
Stefan
^ 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