* Re: [PATCH v8 00/11] Git filter protocol
From: Junio C Hamano @ 2016-10-06 17:17 UTC (permalink / raw)
To: Jeff King
Cc: Lars Schneider, Jakub Narębski, Torsten Bögershausen,
git, Stefan Beller, Martin-Louis Bright, Ramsay Jones
In-Reply-To: <20161006160104.nhpxhfhketr4rb4h@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I am not sure if I have followed all of this discussion, but I am of the
> opinion that Git should not be doing any timeouts at all.
> ...
> If this is debugging output of the form "now I am calling wait() on all
> of the filters", without respect to any timeout, that sounds reasonable.
> Though I would argue that run-command should simply trace_printf()
> any processes it is waiting for, which covers _any_ process, not just
> the filters. That seems like a good match for the rest of the GIT_TRACE
> output, which describes how and when we spawn the sub-processes.
Yup, I agree that having trace_printf() report the wait for any
process is the cleanest way to go. As you guessed the reason why
Lars is bringing up "now we are waiting for this filter" is because
I suggested it as a way to encourage users to file bugs when they
see a hung Git. Originally Lars wanted to have a timeout on wait
and after the timeout wanted to kill the process, and because I
really did not want such a random "you are too slow to die, so I'll
send a signal to you and exit myself without making sure you died"
there, I suggested that if we were to have a timeout, that would be
to timeout the wait only to have a chance to tell the user "we are
stuck waiting on this thing" (and then go back to wait), as it would
either be a buggy filter (i.e. the users need to debug their own
filter code) or a buggy use of wait on Git side (i.e. we would want
to hear about such bugs from them).
Without such a "wait with timeout so that we can tell the user", we
can still respond to "my 'git checkout' hangs forever" with "try
running with GIT_TRACE" as you outlined above, so I do not think we
need the timeout.
Thanks for straightening us out.
^ permalink raw reply
* Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist
From: Stefan Beller @ 2016-10-06 17:20 UTC (permalink / raw)
To: Heiko Voigt
Cc: Junio C Hamano, git@vger.kernel.org, Jeff King, Linus Torvalds
In-Reply-To: <20161006092332.GB38550@book.hvoigt.net>
On Thu, Oct 6, 2016 at 2:23 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> On Wed, Oct 05, 2016 at 03:53:25PM +0200, Heiko Voigt wrote:
>> On Tue, Oct 04, 2016 at 02:03:58PM -0700, Stefan Beller wrote:
>> > Jeff,
>> > thanks for the suggestions, both git_path(..) as well as checking the config,
>> > this seems quite readable to me:
>>
>> When reading the discussion I thought the same: What about the
>> "old-style" repositories. I like this one. Checking both locations
>> is nice.
>
> BTW, since it seems we all agree on the direction. Should we add some
> tests?
>
Good call. What do we want to test for?
* Correctness in case of submodules? (just push and get rejected)
I think that is easy to do.
* Performance with [no, lots of] submodules? That seems harder to me.
I'll add a test for the correctness part and resend.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v9 09/14] pkt-line: add packet_write_gently()
From: Junio C Hamano @ 2016-10-06 17:25 UTC (permalink / raw)
To: Lars Schneider; +Cc: git, ramsay, jnareb, j6t, tboegi, peff, mlbright
In-Reply-To: <B1968DDD-7EAA-477B-B088-5F2A83F29C8C@gmail.com>
Lars Schneider <larsxschneider@gmail.com> writes:
> You are right. Would the solution below be acceptable?
> I would like to keep the `packet_size` variable as it eases the rest
> of the function.
>
>
> const size_t packet_size = size + 4;
>
> - if (packet_size > sizeof(packet_write_buffer))
> + if (size > sizeof(packet_write_buffer) - 4)
> return error("packet write failed - data exceeds max packet size");
Sounds fine; packet_size may have invalid value if size is large
enough but in such a case the function would return without using
it, so no harm is expected, I would say. I'd prefer to see the
definition of packet_size separate from the assignment of size + 4
to it in a case like this, though.
^ permalink raw reply
* Re: [PATCH 5/6] Documentation/git-merge.txt: improve short description in DESCRIPTION
From: Junio C Hamano @ 2016-10-06 17:46 UTC (permalink / raw)
To: Sergey Organov; +Cc: git
In-Reply-To: <87mvihpsla.fsf@javad.com>
Sergey Organov <sorganov@gmail.com> writes:
>>> Last, if "reference" is not good enough and we get to internals anyway,
>>> why not say SHA1 then?
>>
>> Because that is still colloquial? I think s/name/object name/ is a
>> sensible change, but not s/name/reference/.
>
> No, "reference" is more sensible here than any of "name", "object name",
> or "SHA-1", the same way as here:
>
> $ git help glossary
> [...]
> chain
> A list of objects, where each object in the list contains a
> reference to its successor (for example, the successor of a
> commit could be one of its parents).
> [...]
The entry for "chain" and the description under discussion have
stress on different aspect, though. The description of "chain" is
more general: an object refers to another object by referring to it,
by unspecified means. The reason why it is left unspecified is
because the way a tree object refers to blobs and trees is different
from the way a commit object refers to its parents (the former has
object names of blobs and trees in the tree entries; the latter uses
"parent" entries in the object header part to record object names of
parent commits). It wants to stress more on the fact that there is
some mechanism to associate one object to others, than how that
association/linkage is expressed.
The way the resulting commit is described in the original text of
"git merge" description stresses more on "how" by being a lot more
specific to commit objects. It does not just say "refers to parents
(by unspecified means)"; instead it tries to say what exactly are
recorded, i.e. the parents are referred to by recording the object
names of them in a new commit object. It stresses more on "how"
(because it can afford to be more specific, unlike the description
of more general concept of a "chain").
It may be debatable if we want to give the description of what is
exactly recorded at that point of the document, but I personally
think that the users deserve a chance to learn how a merge is
recorded in "git merge" documentation.
^ permalink raw reply
* Re: [PATCH 6/6] Documentation/git-merge.txt: get rid of irrelevant references to git-pull
From: Junio C Hamano @ 2016-10-06 18:06 UTC (permalink / raw)
To: Sergey Organov; +Cc: git
In-Reply-To: <87h98pps7a.fsf@javad.com>
Sergey Organov <sorganov@gmail.com> writes:
> Ah, I now see. I tried to keep the text intact as much as possible, and
> only split it into description and a note. Well, how about this then:
Much better than your earlier patch, but I am not sure if the
updated one is that much better compared to the original.
The pre- and post- state of this "how about this" patch essentially
say the same thing, and I suspect that the primary reason why you
think the post- state is easier to read is because you wrote it,
while the reason why I do not see much difference is because I
didn't write the updated one ;-).
I do find "In this case, ... store the combined history" in the
original a bit awkward to read, but most of that awkardness is
inherited by the updated text. It may benefit from hinting why a
new commit is not needed a bit stronger. Here is my attempt:
When the commit we are merging is a descendant of the current
HEAD, the history leading to the named commit can be, and by
default is, taken as the combined history of the two. Our
history is "fast forwarded" to their history by updating `HEAD`
along with the index to point at the named commit.
This often happens when you are following along somebody else's
work via "git pull" without doing your own development.
I think the awkwardness I felt in the original and your version is
gone from the above attempt, but I doubt that it is better over
either of them in any other way.
> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index b758d55..479400f 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -135,15 +135,17 @@ will exit early with the message "Already up-to-date."
> FAST-FORWARD MERGE
> ------------------
>
> -Often the current branch head is an ancestor of the named commit.
> -This is the most common case especially when invoked from 'git
> -pull': you are tracking an upstream repository, you have committed
> -no local changes, and now you want to update to a newer upstream
> -revision. In this case, a new commit is not needed to store the
> +When current branch head is an ancestor of the named commit,
> +a new commit is not needed to store the
> combined history; instead, the `HEAD` (along with the index) is
> updated to point at the named commit, without creating an extra
> merge commit.
>
> +This is very common case when 'git merge' is invoked from 'git
> +pull': you are tracking an upstream repository, you have committed
> +no local changes, and now you want to update to a newer upstream
> +revision.
> +
> This behavior can be suppressed with the `--no-ff` option.
>
> TRUE MERGE
>
> -- Sergey
^ permalink raw reply
* Re: [PATCH] push: Re-include "push.default=tracking" in the documentation
From: Junio C Hamano @ 2016-10-06 18:24 UTC (permalink / raw)
To: Matthieu Moy
Cc: Ævar Arnfjörð Bjarmason, Git, Johan Herland,
Ramkumar Ramachandra
In-Reply-To: <vpqlgy1h28g.fsf@anie.imag.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> Junio, looks like from the 2013 discussion that you preferred just
>> having that mention in parenthesis instead of its own item, how about
>> just re-applying your fa23348 (with conflicts resolved)?
>
> (fa23348 did this:
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1749,7 +1749,8 @@ push.default::
> +
> This is currently the default, but Git 2.0 will change the default
> to `simple`.
> -* `upstream` - push the current branch to its upstream branch.
> +* `upstream` - push the current branch to its upstream branch
> + (`tracking` is a deprecated synonym for this).
> )
>
> I agree that doing the same thing is the best option.
Sorry, I wasn't paying attention to this thread.
It seems that 87a70e4ce8 ("config doc: rewrite push.default
section", 2013-06-19) removed that mention by accident? The log
message of the commit does not say it actively wanted to remove
mention of `tracking` and/or why it wanted to do so, so I agree that
resurrecting that parenthetical mention is the easiest course of
action at this point.
However.
With today's description of push.default choices, each of which is a
full fledged paragraph, I no longer have the objection I had in
https://public-inbox.org/git/7vip6dgmx2.fsf@alter.siamese.dyndns.org/
against having `tracking` as a separate bullet item. If we add
* `tracking` - a deprecated synonym for `upstream`; do not use this.
to today's list, it would stand out as something different from
others and it will not cause the confusion I feared in the
discussion we had in early 2013. As Jonathan Nieder argued in the
thread back then, having it as one of the bullet point would help
people locate it without using "search" \C-s or / feature.
^ permalink raw reply
* Re: Systems with old regex system headers/libraries don't pick up git's compat/regex header file
From: Junio C Hamano @ 2016-10-06 18:25 UTC (permalink / raw)
To: Richard Lloyd; +Cc: git
In-Reply-To: <9f43a2f1-5d7e-3a2e-5a83-40e92ab0d7b5@connectinternetsolutions.com>
Richard Lloyd <richard.lloyd@connectinternetsolutions.com> writes:
> git ships with a compat/regex tree containing a recent regex
> release, presumably with the intention of allowing systems with
> either no regex or an old regex installed to use the newer compat
> version.
Quick question. Does NO_REGEX help? cf. Makefile
# Define NO_REGEX if your C library lacks regex support with REG_STARTEND
# feature.
^ permalink raw reply
* Re: [PATCH v2 20/25] sequencer: left-trim lines read from the script
From: Junio C Hamano @ 2016-10-06 18:41 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Johannes Schindelin, git, Jakub Narębski
In-Reply-To: <5ed8aaea-9fbe-11b0-49b7-1b42567d4918@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Let me take the opportunity to say
>
> Thank You Very Much!
>
> for the new implementation of rebase -i. I'm using your branch
> rebase-i-extra, and it is such a pleasure to work with on Windows
> compared to the old fully scripted version.
Thanks for testing.
Having more guinea pigs ^W ^W testers before the series is reviewed
here would be a nice boost and would hopefully encourage more
reviewers to help the series into a good shape to be upstreamed ;-)
> ---- 8< ----
> [PATCH] sequencer: strip CR from the end of exec insns
>
> It is not unheard of that editors on Windows write CRLF even if the file
> originally had only LF. This is particularly awkward for exec lines of a
> rebase -i todo sheet. Take for example the insn "exec echo": The shell
> script parser (either of the sequencer or of the shell that is invoked,
> I do not know) splits at the LF and leaves the CR attached to "echo",
> which leads to the unknown command "echo\r".
Interesting find. So it's not just ltrim is being lenient only to
end-user typo, but not doing rtrim can actively hurt ;-)
> Work it around by stripping CR before the command specified with exec is
> passed to the shell.
Makes sense.
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Junio C Hamano @ 2016-10-06 18:42 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <20161006114124.4966-1-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Throwing something at the mailing list to see if anybody is
> interested.
>
> Current '!' aliases move cwd to $GIT_WORK_TREE first, which could make
> handling path arguments hard because they are relative to the original
> cwd. We set GIT_PREFIX to work around it, but I still think it's more
> natural to keep cwd where it is.
>
> We have a way to do that now after 441981b (git: simplify environment
> save/restore logic - 2016-01-26). It's just a matter of choosing the
> right syntax. I'm going with '!!'. I'm not very happy with it. But I
> do like this type of alias.
I do not know why you are not happy with the syntax, but I
personally think it brilliant, both the idea and the preliminary
clean-up that made this possible with a simple patch like this.
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> git.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/git.c b/git.c
> index 296857a..4c1dcf4 100644
> --- a/git.c
> +++ b/git.c
> @@ -252,6 +252,10 @@ static int handle_alias(int *argcp, const char ***argv)
>
> alias_string++;
> commit_pager_choice();
> + if (*alias_string == '!') {
> + keep_cwd = 0;
> + alias_string++;
> + }
> restore_env(keep_cwd);
>
> child.use_shell = 1;
^ permalink raw reply
* Re: Format for %d includes unwanted preceding space
From: Junio C Hamano @ 2016-10-06 18:49 UTC (permalink / raw)
To: Ravi (Tom) Hale; +Cc: git
In-Reply-To: <a3eed8fb-505c-e9c1-6fb6-944d9df2f15d@hale.ee>
"Ravi (Tom) Hale" <ravi@hale.ee> writes:
> The log --format="%d" includes preceding space:
>
> $ git log -n1 --format="XX%dXX" HEAD
> XX (HEAD -> master)XX
>
> I know of no other %<token> that is output in this way.
That is primarily because %d predates %[-+ ]<placeholder> mechanism,
I think. It is too late to redefine what "%d" means and force all
users to update their script to use "% d" instead, unfortunately.
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Jeff King @ 2016-10-06 19:00 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <20161006114124.4966-1-pclouds@gmail.com>
On Thu, Oct 06, 2016 at 06:41:24PM +0700, Nguyễn Thái Ngọc Duy wrote:
> Throwing something at the mailing list to see if anybody is
> interested.
>
> Current '!' aliases move cwd to $GIT_WORK_TREE first, which could make
> handling path arguments hard because they are relative to the original
> cwd. We set GIT_PREFIX to work around it, but I still think it's more
> natural to keep cwd where it is.
>
> We have a way to do that now after 441981b (git: simplify environment
> save/restore logic - 2016-01-26). It's just a matter of choosing the
> right syntax. I'm going with '!!'. I'm not very happy with it. But I
> do like this type of alias.
Hmm. I wonder if any commands will be fooled by not being moved to
GIT_WORK_TREE. I know that there is a bug already in this case:
export HOME=$PWD
for i in one two; do
git init $i &&
(cd $i &&
echo "* diff=$i" >.gitattributes &&
git add . &&
git commit -m $i) &&
git config --global diff.$i.textconv "sed s/^/$i:/"
done
cd two &&
git --git-dir=$(pwd)/../one/.git --work-tree=$(pwd)/../one show
This shows the contents of repo one using the .gitattributes from repo
two. I am not sure if the bug is that when GIT_WORK_TREE is set,
git-show doesn't position itself at the toplevel of that tree, or if the
attributes machinery should be more careful about looking up paths in
the work-tree versus the current directory.
This bug is tangential to your patch (it has nothing to do with aliases,
and should be fixed regardless). But I wonder if your "!!" aliases will
expose this bug more frequently.
-Peff
PS I think your "!!" syntax conflicts with something like:
git -c alias.has-changes="!! git diff --quiet' has-changes
I don't know if that is worth worrying about or not. I also notice
that using "!!git diff" with no space seems broken, as it seems to
skip using the shell. I wonder if that is a bug in run-command.
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Jeff King @ 2016-10-06 19:07 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <20161006190014.owmqr2eqyk5h5cau@sigill.intra.peff.net>
On Thu, Oct 06, 2016 at 03:00:14PM -0400, Jeff King wrote:
> PS I think your "!!" syntax conflicts with something like:
>
> git -c alias.has-changes='!! git diff --quiet' has-changes
>
> I don't know if that is worth worrying about or not. I also notice
> that using "!!git diff" with no space seems broken, as it seems to
> skip using the shell. I wonder if that is a bug in run-command.
Nevermind this last bit. It is the shell that is complaining
(rightfully) about "!git"; the error message is just confusing because
it mentions $0, which contains the whole script:
$ git -c alias.has-changes='!!git diff --quiet' has-changes
!git diff --quiet: 1: !git diff --quiet: !git: not found
The "!! git diff" syntax sill has the conflict I mentioned (though note
I screwed up the quotes in what I wrote above).
-Peff
^ permalink raw reply
* Re: Systems with old regex system headers/libraries don't pick up git's compat/regex header file
From: Jeff King @ 2016-10-06 19:11 UTC (permalink / raw)
To: Richard Lloyd; +Cc: git
In-Reply-To: <9f43a2f1-5d7e-3a2e-5a83-40e92ab0d7b5@connectinternetsolutions.com>
On Thu, Oct 06, 2016 at 10:15:49AM +0100, Richard Lloyd wrote:
> Unfortunately, on systems with an older regex shipped as
> standard (e.g. HP-UX 11), the include path picks up
> /usr/include/regex.h first, which doesn't define REG_STARTEND
> and the git-compat-util.h check fails.
>
> The fix I applied on HP-UX 11 was to add -Icompat/regex
> to the CFLAGS ahead of other -I directives. Another possible
> change needed might be to line 69 of compat/regex/regex.c:
Junio mentioned the NO_REGEX knob in the Makefile. If that works for
you, the next step is probably to add a line to the HP-UX section of
config.mak.uname, so that it just works out of the box.
-Peff
^ permalink raw reply
* Re: [PATCH 1/3] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"
From: Junio C Hamano @ 2016-10-06 19:15 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List
In-Reply-To: <CACsJy8D7c8Z_ugasn_scf391+C6GxJp1CYwHY4ndvVtLiJzxnQ@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> 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 definitely shouldn't have said that, especially "waste". Many
issues around i-t-a and diff make my head hurt when I think about
them [*1*], but not wanting to spend time that gets my
head hurt and not wanting to waste time are totally different
things. My apologies.
I missed something curious in your statement above, i.e. "external
diff". I thought we have pretty much got rid of all the invocation
of "git diff" via the run_command() interface and we do not need the
command line option (we only need the options->shift_ita so that
callers like "git status" can seletively ask for it when making
internal calls), and that is why I didn't want to see it.
> 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.
It is way too late to revert 3f6d56d. Even though I think it was
overall a mistake to treat i-t-a as "not exist" while committing,
people are now used to the behaviour with that change, and we need
to make our progress within the constraint of the real world.
[Footnote]
Here is one of the things around i-t-a and diff. If you make "git
diff" (between the index and the working tree) report "new" file, it
would imply that "git apply" run without "--index" should create an
ita entry in the index for symmetry, wouldn't it? That by itself
can be seen as an improvement (we no longer would have to say that
"git apply patchfile && git commit -a" that is run in a clean state
will forget new files the patchfile creates), but it also means we
now need a repository in order to run "git apply" (without "--index"),
which is a problem, as "git apply" is often used as a better "patch".
"git apply --cached" may also become "interesting". A patch that
would apply cleanly to HEAD should apply cleanly if you did this:
$ git read-tree HEAD
$ git apply --cached <patch
no matter what the working tree state is. Should a patch that
creates a "new" file add contents to the index, or just an i-t-a
entry? I could argue it both ways, but either is quite satisfactory
and makes my head hurt.
^ permalink raw reply
* Re: Regression: git no longer works with musl libc's regex impl
From: Ævar Arnfjörð Bjarmason @ 2016-10-06 19:18 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Rich Felker, Jeff King, Git, musl
In-Reply-To: <alpine.DEB.2.20.1610041802310.35196@virtualbox>
On Tue, Oct 4, 2016 at 6:08 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> 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.
But just to clarify, does anyone have any objection to making our
configure.ac compile a C program to check for this sort of thing?
Because that seems like the easiest solution to this class of problem.
^ permalink raw reply
* Re: [PATCH v2 05/25] sequencer: allow the sequencer to take custody of malloc()ed data
From: Junio C Hamano @ 2016-10-06 19:23 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <alpine.DEB.2.20.1610051338530.35196@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> If you prefer to accept such sloppy work, I will change it of course,
> feeling dirty that it has my name on it.
I do want neither leaks nor sloppyness, and I thought that was
understood by everybody, hence I thought the last round made it
clear that _entrust() must not exist.
Let's try again.
We manage lifetime of a field in a structure in one of three ways in
our codebase [*1*].
* A field can always point at a borrowed piece of memory that the
destruction of the containing structure or re-assignment to the
field do not have to free the current value of the field. This
is a minority, simply because it is rare for a field to be always
able to borrow from others. The destruction of the containing
structure or re-assignment to the field needs to do nothing
special.
* A field can own the piece of memory that it points at. The
destruction of the containing structure or re-assignment to the
field needs to free the current value of the field [*2*]. A
field that can be assigned a value from the configuration (which
is typically xstrdup()'ed) is an example of such a field, and if
a command line option also can assign to the field, we'd need to
xstrdup() it, even though we know we could borrow from argv[],
because cleaning-up needs to be able to free(3) it.
* A field can sometimes own and sometimes borrow the memory, and it
is accompanied by another field to tell which case it is, so that
cleaning-up can tell when it needs to be free(3)d. This is a
minority case, and we generally avoid it especially in modern
code for small allocation, as it makes the lifetime rule more
complex than it is worth.
The _entrust() thing may have been an excellent fourth option if it
were cost-free. xstrdup() that the second approach necessitates for
literal strings (like part of argv[]) would become unnecessary and
we can treat as if all the fields in a structure were all borrowing
the memory from elsewhere (in fact, _entrust() creates the missing
owners of pieces of memory that need to be freed later); essentially
it turns a "mixed ownership" case into the first approach.
But it is not cost-free. The mechanism needs to allocate memory to
become the missing owner and keep track of which pieces of memory
need to be freed later, and the resulting code does not become any
easier to follow. The programmer still needs to know which ones to
_entrust() just like the programmer in the model #2 needs to make
sure xstrdup() is done for appropriate pieces of memory--the only
difference is that an _entrust() programmer needs to mark the pieces
of memory to be freed, while a #2 programmer needs to xstrdup() the
pieces of memory that are being "borrowed".
So let's not add the fourth way to our codebase. "mixed ownership"
case should be turned into "field owns the memory", i.e. approach #2.
[Footnotes]
*1* It is normal for different fields in the same structure follow
different lifetime rules.
*2* Unless leaks are allowed, that is. I think we have instances
where "git cmd --opt=A --opt=B" allocates and stores a new piece
of memory that is computed based on A and store it to a field,
and then overwrites the field with another allocation of a value
based on B without freeing old value, saying "the caller can
avoid passing the same thing twice, and such a leak is miniscule
anyway". That is not nice, and we've been reducing them over
time.
^ permalink raw reply
* Re: Regression: git no longer works with musl libc's regex impl
From: Jeff King @ 2016-10-06 19:23 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Johannes Schindelin, Rich Felker, Git, musl
In-Reply-To: <CACBZZX4XPqZauD_M_ieOwVauT1fi3MQb4+6taELQaRG9M-Kz_w@mail.gmail.com>
On Thu, Oct 06, 2016 at 09:18:29PM +0200, Ævar Arnfjörð Bjarmason wrote:
> On Tue, Oct 4, 2016 at 6:08 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > 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.
>
> But just to clarify, does anyone have any objection to making our
> configure.ac compile a C program to check for this sort of thing?
> Because that seems like the easiest solution to this class of problem.
No, I think that is the exact purpose of configure.ac and autoconf.
It would be neat if we could auto-fallback during the build. Rich
suggested always compiling compat/regex.c, and just having it be a noop
at the preprocessor level. I'm not sure if that would work, though,
because we'd have to include the system "regex.h" to know if we have
REG_STARTEND, at which point it is potentially too late to compile our
own regex routines (we're potentially going to conflict with the system
declarations).
-Peff
^ permalink raw reply
* Re: [RFC/PATCH 0/2] place cherry pick line below commit title
From: Junio C Hamano @ 2016-10-06 19:24 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, Christian Couder
In-Reply-To: <b0e43079-498a-469b-c4b6-f4fc2b8e2aa4@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> How important is this feature? It doesn't seem too difficult to add,
> although it does break compatibility (in particular, "--signoff" must
> now be documented as "after the last trailer" instead of "at the end
> of the commit message").
The sign-off has always been meant to be appended at the end of the
trailer block, so there is no "compatibility" issue. It's just how
you phrase the explanation of the behaviour, I would think.
^ permalink raw reply
* Re: Regression: git no longer works with musl libc's regex impl
From: Rich Felker @ 2016-10-06 19:25 UTC (permalink / raw)
To: Jeff King
Cc: Ævar Arnfjörð Bjarmason, Johannes Schindelin, Git,
musl
In-Reply-To: <20161006192339.3yddgxxk7jn7zfqx@sigill.intra.peff.net>
On Thu, Oct 06, 2016 at 03:23:40PM -0400, Jeff King wrote:
> On Thu, Oct 06, 2016 at 09:18:29PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> > On Tue, Oct 4, 2016 at 6:08 PM, Johannes Schindelin
> > <Johannes.Schindelin@gmx.de> wrote:
> > > 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.
> >
> > But just to clarify, does anyone have any objection to making our
> > configure.ac compile a C program to check for this sort of thing?
> > Because that seems like the easiest solution to this class of problem.
>
> No, I think that is the exact purpose of configure.ac and autoconf.
>
> It would be neat if we could auto-fallback during the build. Rich
> suggested always compiling compat/regex.c, and just having it be a noop
> at the preprocessor level. I'm not sure if that would work, though,
> because we'd have to include the system "regex.h" to know if we have
> REG_STARTEND, at which point it is potentially too late to compile our
> own regex routines (we're potentially going to conflict with the system
> declarations).
If you have autoconf testing for REG_STARTEND at configure time then
compat/regex.c can #include "config.h" and test for HAVE_REG_STARTEND
rather than for REG_STARTEND, or something like that.
Rich
^ permalink raw reply
* Re: Regression: git no longer works with musl libc's regex impl
From: Jeff King @ 2016-10-06 19:28 UTC (permalink / raw)
To: Rich Felker
Cc: Ævar Arnfjörð Bjarmason, Johannes Schindelin, Git,
musl
In-Reply-To: <20161006192500.GS19318@brightrain.aerifal.cx>
On Thu, Oct 06, 2016 at 03:25:00PM -0400, Rich Felker wrote:
> > No, I think that is the exact purpose of configure.ac and autoconf.
> >
> > It would be neat if we could auto-fallback during the build. Rich
> > suggested always compiling compat/regex.c, and just having it be a noop
> > at the preprocessor level. I'm not sure if that would work, though,
> > because we'd have to include the system "regex.h" to know if we have
> > REG_STARTEND, at which point it is potentially too late to compile our
> > own regex routines (we're potentially going to conflict with the system
> > declarations).
>
> If you have autoconf testing for REG_STARTEND at configure time then
> compat/regex.c can #include "config.h" and test for HAVE_REG_STARTEND
> rather than for REG_STARTEND, or something like that.
Right, that part is easy; we do not even have to touch compat/regex.c,
because we already have such a knob in the Makefile (NO_REGEX), and
autoconf just needs to tweak that knob.
My question was whether we could do it without running a separate
compile (via autoconf or via the Makefile), and I think the answer is
"no".
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] files_read_raw_ref: avoid infinite loop on broken symlinks
From: Johannes Sixt @ 2016-10-06 19:31 UTC (permalink / raw)
To: Jeff King; +Cc: git, Michael Haggerty
In-Reply-To: <20161006164825.otms5ovz2vzanimw@sigill.intra.peff.net>
Am 06.10.2016 um 18:48 schrieb Jeff King:
> +test_expect_success SYMLINKS 'ref resolution not confused by broken symlinks' '
> + ln -s does-not-exist .git/broken &&
> + test_must_fail git rev-parse --verify broken
Hm, lower-case named refs directly in .git are frowned upon, no? If we
ever decide to forbid them outright, this ref-parse might still fail,
but for the wrong reason. Should you not better pick an example below ref/?
-- Hannes
^ permalink raw reply
* [PATCH 0/2] submodule pushes be extra careful
From: Stefan Beller @ 2016-10-06 19:37 UTC (permalink / raw)
To: gitster; +Cc: git, hvoigt, torvalds, peff, Stefan Beller
This is a reroll of
http://public-inbox.org/git/20161004210359.15266-1-sbeller@google.com/
([PATCHv3 1/2] push: change submodule default to check when submodules exist)
but with a test.
As we only have a heuristic, the test failed initially as these tests don't
have any configuration at all nor do they have the submodule repos in the
superprojects git dir. So I was looking for a cheap way to add a config.
I could have written the config myself via git config, I think it is more
idiomatic to use a submodule command for that. However just getting the
configuration added is not possible with a submodule command, so I made one
in the first patch.
Thanks,
Stefan
Stefan Beller (2):
submodule add: extend force flag to add existing repos
push: change submodule default to check when submodules exist
builtin/push.c | 15 ++++++++++++++-
git-submodule.sh | 10 ++++++++--
t/t5531-deep-submodule-push.sh | 6 +++++-
t/t7400-submodule-basic.sh | 14 ++++++++++++++
4 files changed, 41 insertions(+), 4 deletions(-)
--
2.10.1.353.g1629400
^ permalink raw reply
* [PATCH 1/2] submodule add: extend force flag to add existing repos
From: Stefan Beller @ 2016-10-06 19:37 UTC (permalink / raw)
To: gitster; +Cc: git, hvoigt, torvalds, peff, Stefan Beller
In-Reply-To: <20161006193725.31553-1-sbeller@google.com>
Currently the force flag in `git submodule add` takes care of possibly
ignored files or when a name collision occurs.
However there is another situation where submodule add comes in handy:
When you already have a gitlink recorded, but no configuration was
done (i.e. no .gitmodules file nor any entry in .git/config) and you
want to generate these config entries. For this situation allow
`git submodule add` to proceed if there is already a submodule at the
given path in the index.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
git-submodule.sh | 10 ++++++++--
t/t7400-submodule-basic.sh | 14 ++++++++++++++
2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index a024a13..3762616 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -207,8 +207,14 @@ cmd_add()
tstart
s|/*$||
')
- git ls-files --error-unmatch "$sm_path" > /dev/null 2>&1 &&
- die "$(eval_gettext "'\$sm_path' already exists in the index")"
+ if test -z "$force"
+ then
+ git ls-files --error-unmatch "$sm_path" > /dev/null 2>&1 &&
+ die "$(eval_gettext "'\$sm_path' already exists in the index")"
+ else
+ git ls-files -s "$sm_path" | sane_grep -v "^160000" > /dev/null 2>&1 &&
+ die "$(eval_gettext "'\$sm_path' already exists in the index and is not a submodule")"
+ fi
if test -z "$force" && ! git add --dry-run --ignore-missing "$sm_path" > /dev/null 2>&1
then
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index b77cce8..c09ce0d 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -152,6 +152,20 @@ test_expect_success 'submodule add to .gitignored path with --force' '
)
'
+test_expect_success 'submodule add to reconfigure existing submodule with --force' '
+ (
+ cd addtest-ignore &&
+ git submodule add --force bogus-url submod &&
+ git submodule add -b initial "$submodurl" submod-branch &&
+ test "bogus-url" = "$(git config -f .gitmodules submodule.submod.url)" &&
+ test "bogus-url" = "$(git config submodule.submod.url)" &&
+ # Restore the url
+ git submodule add --force "$submodurl" submod
+ test "$submodurl" = "$(git config -f .gitmodules submodule.submod.url)" &&
+ test "$submodurl" = "$(git config submodule.submod.url)"
+ )
+'
+
test_expect_success 'submodule add --branch' '
echo "refs/heads/initial" >expect-head &&
cat <<-\EOF >expect-heads &&
--
2.10.1.353.g1629400
^ permalink raw reply related
* [PATCHv4 2/2] push: change submodule default to check when submodules exist
From: Stefan Beller @ 2016-10-06 19:37 UTC (permalink / raw)
To: gitster; +Cc: git, hvoigt, torvalds, peff, Stefan Beller
In-Reply-To: <20161006193725.31553-1-sbeller@google.com>
When working with submodules, it is easy to forget to push the submodules.
The setting 'check', which checks if any existing submodule is present on
at least one remote of the submodule remotes, is designed to prevent this
mistake.
Flipping the default to check for submodules is safer than the current
default of ignoring submodules while pushing.
However checking for submodules requires additional work[1], which annoys
users that do not use submodules, so we turn on the check for submodules
based on a cheap heuristic, the existence of the .git/modules directory.
That directory doesn't exist when no submodules are used and is only
created and populated when submodules are cloned/added.
When the submodule directory doesn't exist, a user may have changed the
gitlinks via plumbing commands. Currently the default is to not check.
RECURSE_SUBMODULES_DEFAULT is effectively RECURSE_SUBMODULES_OFF currently,
though it may change in the future. When no submodules exist such a check
is pointless as it would fail anyway, so let's just turn it off.
[1] https://public-inbox.org/git/CA+55aFyos78qODyw57V=w13Ux5-8SvBqObJFAq22K+XKPWVbAA@mail.gmail.com/
Signed-off-by: Stefan Beller <sbeller@google.com>
---
builtin/push.c | 15 ++++++++++++++-
t/t5531-deep-submodule-push.sh | 6 +++++-
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/builtin/push.c b/builtin/push.c
index 3bb9d6b..683f270 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -3,6 +3,7 @@
*/
#include "cache.h"
#include "refs.h"
+#include "dir.h"
#include "run-command.h"
#include "builtin.h"
#include "remote.h"
@@ -22,6 +23,7 @@ static int deleterefs;
static const char *receivepack;
static int verbosity;
static int progress = -1;
+static int has_submodules_configured;
static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
static enum transport_family family;
@@ -31,6 +33,15 @@ static const char **refspec;
static int refspec_nr;
static int refspec_alloc;
+static void preset_submodule_default(void)
+{
+ if (has_submodules_configured || file_exists(git_path("modules")) ||
+ (!is_bare_repository() && file_exists(".gitmodules")))
+ recurse_submodules = RECURSE_SUBMODULES_CHECK;
+ else
+ recurse_submodules = RECURSE_SUBMODULES_OFF;
+}
+
static void add_refspec(const char *ref)
{
refspec_nr++;
@@ -495,7 +506,8 @@ static int git_push_config(const char *k, const char *v, void *cb)
const char *value;
if (!git_config_get_value("push.recursesubmodules", &value))
recurse_submodules = parse_push_recurse_submodules_arg(k, value);
- }
+ } else if (starts_with(k, "submodule.") && ends_with(k, ".url"))
+ has_submodules_configured = 1;
return git_default_config(k, v, NULL);
}
@@ -552,6 +564,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
};
packet_trace_identity("push");
+ preset_submodule_default();
git_config(git_push_config, &flags);
argc = parse_options(argc, argv, prefix, options, push_usage, 0);
set_push_cert_flags(&flags, push_cert);
diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index 198ce84..e690749 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh
@@ -65,7 +65,11 @@ test_expect_success 'push fails if submodule commit not on remote' '
git add gar/bage &&
git commit -m "Third commit for gar/bage" &&
# the push should fail with --recurse-submodules=check
- # on the command line...
+ # on the command line. "check" is the default for repos in
+ # which submodules are detected by existence of config,
+ # .gitmodules file or an internal .git/modules/<submodule-repo>
+ git submodule add -f ../submodule.git gar/bage &&
+ test_must_fail git push ../pub.git master &&
test_must_fail git push --recurse-submodules=check ../pub.git master &&
# ...or if specified in the configuration..
--
2.10.1.353.g1629400
^ permalink raw reply related
* [PATCH v2 1/2] files_read_raw_ref: avoid infinite loop on broken symlinks
From: Jeff King @ 2016-10-06 19:41 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git, Michael Haggerty
In-Reply-To: <1c39b371-eb41-05d9-3c48-bd41764c9c9a@kdbg.org>
On Thu, Oct 06, 2016 at 09:31:22PM +0200, Johannes Sixt wrote:
> Am 06.10.2016 um 18:48 schrieb Jeff King:
> > +test_expect_success SYMLINKS 'ref resolution not confused by broken symlinks' '
> > + ln -s does-not-exist .git/broken &&
> > + test_must_fail git rev-parse --verify broken
>
> Hm, lower-case named refs directly in .git are frowned upon, no? If we ever
> decide to forbid them outright, this ref-parse might still fail, but for the
> wrong reason. Should you not better pick an example below ref/?
I suppose so. The test case was adapted from a real-world example, but
it fails equally well with .git/refs/heads/broken. The only restriction
is that the symlink _destination_ cannot look like "refs/heads/...".
Here's a replacement 1/2. The second patch remains unchanged.
-- >8 --
Subject: files_read_raw_ref: avoid infinite loop on broken symlinks
Our ref resolution first runs lstat() on any path we try to
look up, because we want to treat symlinks specially (by
resolving them manually and considering them symrefs). But
if the results of `readlink` do _not_ look like a ref, we
fall through to treating it like a normal file, and just
read the contents of the linked path.
Since fcb7c76 (resolve_ref_unsafe(): close race condition
reading loose refs, 2013-06-19), that "normal file" code
path will stat() the file and if we see ENOENT, will jump
back to the lstat(), thinking we've seen inconsistent
results between the two calls. But for a symbolic ref, this
isn't a race: the lstat() found the symlink, and the stat()
is looking at the path it points to. We end up in an
infinite loop calling lstat() and stat().
We can fix this by avoiding the retry-on-inconsistent jump
when we know that we found a symlink. While we're at it,
let's add a comment explaining why the symlink case gets to
this code in the first place; without that, it is not
obvious that the correct solution isn't to avoid the stat()
code path entirely.
Signed-off-by: Jeff King <peff@peff.net>
---
refs/files-backend.c | 7 ++++++-
t/t1503-rev-parse-verify.sh | 5 +++++
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0709f60..d826557 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1403,6 +1403,11 @@ static int files_read_raw_ref(struct ref_store *ref_store,
ret = 0;
goto out;
}
+ /*
+ * It doesn't look like a refname; fall through to just
+ * treating it like a non-symlink, and reading whatever it
+ * points to.
+ */
}
/* Is it a directory? */
@@ -1426,7 +1431,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,
*/
fd = open(path, O_RDONLY);
if (fd < 0) {
- if (errno == ENOENT)
+ if (errno == ENOENT && !S_ISLNK(st.st_mode))
/* inconsistent with lstat; retry */
goto stat_ref;
else
diff --git a/t/t1503-rev-parse-verify.sh b/t/t1503-rev-parse-verify.sh
index ab27d0d..492edff 100755
--- a/t/t1503-rev-parse-verify.sh
+++ b/t/t1503-rev-parse-verify.sh
@@ -139,4 +139,9 @@ test_expect_success 'master@{n} for various n' '
test_must_fail git rev-parse --verify master@{$Np1}
'
+test_expect_success SYMLINKS 'ref resolution not confused by broken symlinks' '
+ ln -s does-not-exist .git/refs/heads/broken &&
+ test_must_fail git rev-parse --verify broken
+'
+
test_done
--
2.10.1.506.g904834d
^ permalink raw reply related
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