* [PATCH] clean up confusing suggestion for commit references
From: Heiko Voigt @ 2016-10-07 9:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
The description for referencing commits looks as if it is contradicting
the example, since it is itself enclosed in double quotes. Lets use
single quotes around the description and include the double quotes in
the description so it matches the example.
---
Sorry for opening this up again but I just looked up the format and was
like: "Umm, which one is now the correct one..."
For this makes more sense. What do others think?
Cheers Heiko
Documentation/SubmittingPatches | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 08352de..692f4ce 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -122,7 +122,7 @@ without external resources. Instead of giving a URL to a mailing list
archive, summarize the relevant points of the discussion.
If you want to reference a previous commit in the history of a stable
-branch, use the format "abbreviated sha1 (subject, date)",
+branch, use the format 'abbreviated sha1 ("subject", date)',
with the subject enclosed in a pair of double-quotes, like this:
Commit f86a374 ("pack-bitmap.c: fix a memleak", 2015-03-30)
--
2.10.0.645.g54f1e86
^ permalink raw reply related
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Johannes Schindelin @ 2016-10-07 11:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Nguyễn Thái Ngọc Duy, git
In-Reply-To: <xmqq60p5l3om.fsf@gitster.mtv.corp.google.com>
[-- Attachment #1: Type: text/plain, Size: 1117 bytes --]
Hi Junio,
On Thu, 6 Oct 2016, Junio C Hamano wrote:
> 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.
I guess he is not happy with it because "!!" is quite unintuitive a
construct. I know that *I* would have been puzzled by it, asking "What the
heck does this do?".
Ciao,
Dscho
^ permalink raw reply
* Re: Regression: git no longer works with musl libc's regex impl
From: Jakub Narębski @ 2016-10-07 11:30 UTC (permalink / raw)
To: Ramsay Jones, Ævar Arnfjörð Bjarmason,
Johannes Schindelin, git
Cc: Rich Felker, Jeff King, musl
In-Reply-To: <20336ac7-a494-d725-f928-834b1b3194fe@ramsayjones.plus.com>
W dniu 07.10.2016 o 00:42, Ramsay Jones pisze:
> On 06/10/16 20:18, Ævar Arnfjörð Bjarmason wrote:
[...]
>> 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.
>
> Err, you do know that we already do that, right?
>
> [see commit a1e3b669 ("autoconf: don't use platform regex if it lacks REG_STARTEND", 17-08-2010)]
>
> In fact, if you run the auto tools on cygwin, you get a different setting
> for NO_REGEX than via config.mak.uname. Which is why I don't run configure
> on cygwin. :-D
>
> [The issue is exposed by t7008-grep-binary.sh, where the cygwin native
> regex library matches '.' in a pattern with the NUL character. ie the
> test_expect_failure test passes.]
Huh. So we have NO_REGEX support in ./configure, and people using
Git on untypical architectures and systems *can* make use of it.
It was just described wrongly, so in turn to have the more neutral
description, the same as in Makefile, let's do this:
-------- >8 ---------- >8 ------------- >8 ---------- >8 ----------
Subject: [PATCH] configure.ac: Improve description of NO_REGEX test
The commit 2f8952250a changed description of NO_REGEX build config
variable to be more neutral, and actually say that it is about
support for REG_STARTEND. Change description in configure.ac to
be the same.
Change also the test message and variable name to match. The test
just checks that REG_STARTEND is #defined.
Issue-found-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Signed-off-by: Jakub Narębski <jnareb@gmail.com>
---
configure.ac | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/configure.ac b/configure.ac
index aa9c91d..7f39fd0 100644
--- a/configure.ac
+++ b/configure.ac
@@ -835,9 +835,10 @@ AC_CHECK_TYPE([struct addrinfo],[
])
GIT_CONF_SUBST([NO_IPV6])
#
-# Define NO_REGEX if you have no or inferior regex support in your C library.
-AC_CACHE_CHECK([whether the platform regex can handle null bytes],
- [ac_cv_c_excellent_regex], [
+# Define NO_REGEX if your C library lacks regex support with REG_STARTEND
+# feature.
+AC_CACHE_CHECK([whether the platform regex supports REG_STARTEND],
+ [ac_cv_c_regex_with_reg_startend], [
AC_EGREP_CPP(yippeeyeswehaveit,
AC_LANG_PROGRAM([AC_INCLUDES_DEFAULT
#include <regex.h>
@@ -846,10 +847,10 @@ AC_EGREP_CPP(yippeeyeswehaveit,
yippeeyeswehaveit
#endif
]),
- [ac_cv_c_excellent_regex=yes],
- [ac_cv_c_excellent_regex=no])
+ [ac_cv_c_regex_with_reg_startend=yes],
+ [ac_cv_c_regex_with_reg_startend=no])
])
-if test $ac_cv_c_excellent_regex = yes; then
+if test $ac_cv_c_regex_with_reg_startend = yes; then
NO_REGEX=
else
NO_REGEX=YesPlease
--
2.10.0
^ permalink raw reply related
* Re: [PATCH 6/6] Documentation/git-merge.txt: get rid of irrelevant references to git-pull
From: Sergey Organov @ 2016-10-07 11:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqmvihl5df.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> 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.
It's not intended to be much better. It is aimed at single simple
target: get rid of git-pull from descriptions of operations of
git-merge.
I'd just remove those git-pull reference, the only one that is left
after the patch, but it looks like git-merge needs an excuse to have
fast-forward on by default, and that excuse is the common git-pull case.
[I'd prefer 'git-merge --ff' were called from 'git-pull' and --no-ff be
the default for git-merge, but that's not the case, so I left the
reference to git-pull intact.]
>
> 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.
This is entirely different matter, and should be a subject of another
patch, if any. My patch meant to only address git-pull references, with
as few changes as possible.
-- Sergey
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Duy Nguyen @ 2016-10-07 12:27 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <alpine.DEB.2.20.1610071319520.35196@virtualbox>
On Fri, Oct 7, 2016 at 6:20 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Junio,
>
> On Thu, 6 Oct 2016, Junio C Hamano wrote:
>
>> 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.
>
> I guess he is not happy with it because "!!" is quite unintuitive a
> construct. I know that *I* would have been puzzled by it, asking "What the
> heck does this do?".
Yep. And I wouldn't want to set a tradition for the next alias type
'!!!'. There's no good choice to represent a new alias type with a
leading symbol. This just occurred to me, however, what do you think
about a new config group for it? With can have something like
externalAlias.* (or some other name) that lives in parallel with
alias.*. Then we don't need '!' (or '!!') at all.
--
Duy
^ permalink raw reply
* Re: [PATCHv3 1/2] push: change submodule default to check when submodules exist
From: Heiko Voigt @ 2016-10-07 12:41 UTC (permalink / raw)
To: Stefan Beller
Cc: Junio C Hamano, git@vger.kernel.org, Jeff King, Linus Torvalds
In-Reply-To: <CAGZ79ka=bVX51wrSiz4u7xOy+54t+v7a-iU5dHtQoNnB7-NAkw@mail.gmail.com>
On Thu, Oct 06, 2016 at 10:20:16AM -0700, Stefan Beller wrote:
> 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.
Well I though about the following tests:
* Without submodules: Make sure submodule processing is disabled by
default
* With new-style submodules: Make sure submodules are processed by
default
* With old-style submodules: Make sure submodules are processed by
default
But I have to admit that I did not think about the "how can we do that".
But performance seems to be the only thing that is exposing the
processing when we have no submodules, so it seems we can only easily do
the tests with submodules.
Cheers Heiko
^ permalink raw reply
* Re: [PATCH 1/2] submodule add: extend force flag to add existing repos
From: Heiko Voigt @ 2016-10-07 12:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Beller, git, torvalds, peff
In-Reply-To: <xmqqshs9i6fr.fsf@gitster.mtv.corp.google.com>
On Thu, Oct 06, 2016 at 01:11:20PM -0700, Junio C Hamano wrote:
> Stefan Beller <sbeller@google.com> writes:
>
> > 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.
Is it important that the submodule is in the index? How about worktree?
From the index entry alone we can not deduce the values anyway. So I
would say the submodule has to be in the worktree, no matter what is in
the index. If its not in the index we can also add it.
BTW, that is the way I imagined submodules would work in the first
place: just clone and add them, like I described here[1].
> > Signed-off-by: Stefan Beller <sbeller@google.com>
> > ---
>
> Yup, the goal makes perfect sense.
>
> I vaguely recall discussing this exact issue of "git submodule add"
> that refuses to add a path that already is a gitlink (via "git add"
> that has previously been run) elsewhere on this list some time ago.
Yes there was a discussion, see the link.
Cheers Heiko
[1] http://public-inbox.org/git/%3C20160916141143.GA47240@book.hvoigt.net%3E/
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Duy Nguyen @ 2016-10-07 13:07 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Johannes Schindelin, Junio C Hamano, Git Mailing List
In-Reply-To: <vpqmvig5ns6.fsf@anie.imag.fr>
On Fri, Oct 7, 2016 at 7:47 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Fri, Oct 7, 2016 at 6:20 PM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>>> Hi Junio,
>>>
>>> On Thu, 6 Oct 2016, Junio C Hamano wrote:
>>>
>>>> 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.
>>>
>>> I guess he is not happy with it because "!!" is quite unintuitive a
>>> construct. I know that *I* would have been puzzled by it, asking
>>> "What the
>>> heck does this do?".
>>
>> Yep. And I wouldn't want to set a tradition for the next alias type
>> '!!!'. There's no good choice to represent a new alias type with a
>> leading symbol. This just occurred to me, however, what do you think
>> about a new config group for it? With can have something like
>> externalAlias.* (or some other name) that lives in parallel with
>> alias.*. Then we don't need '!' (or '!!') at all.
>
> Another possibility: !(nocd), which leaves room
> for !(keyword1,keyword2,...) if needed later. Also, it is consistent
> with the :(word) syntax of pathspecs.
This seems to solve my problem nicely.
--
Duy
^ 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-07 12:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <xmqqwphljnlj.fsf@gitster.mtv.corp.google.com>
On Fri, Oct 7, 2016 at 2:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.
No problem. I do appreciate a straight shoot down though. Many of my
topics have been going on for months (ones not in 'pu') and seeing it
rejected near the end is worse than stopping working on them early.
> 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 don't know if we have had any external diff calls in our shell-based
commands. I don't read them often. Regardless, people do use "git
diff" and it should show the "right thing" (I know it's subjective).
Or at least be consistent with both git-commit and git-status.
> [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
Off topic. This reminds me of an old patch about apply and ita [1] but
that one is not the same here
> 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".
We could detect "no repo available" and ignore the index, I guess.
> "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.
--cached tells you to put new contents in the index. I-ta entries,
being a reminder to add stuff, don't really fit in here because you
want to add contents _now_, i think. After a successful "git apply
--cached", a "git commit" should contain exactly what the applied
patch has. If new files are i-t-a entries instead, then the new commit
would not be the same as the patch.
[1] https://public-inbox.org/git/1451181092-26054-4-git-send-email-pclouds@gmail.com/
--
Duy
^ permalink raw reply
* Re: [PATCH 5/6] Documentation/git-merge.txt: improve short description in DESCRIPTION
From: Sergey Organov @ 2016-10-07 13:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqshs9l69h.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> 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").
That's were our disagreement actually is, and that's what I've tried to
fix with s/name/reference/, and that's why I'm against s/name/object
name/.
Rather than being more (and more) specific at every opportunity, one
needs a good reason to get more specific. In this particular case,
general DAG terminology seems to be enough to describe git-merge
semantics, thus using GIT specifics is unfounded.
> It may be debatable if we want to give the description of what is
> exactly recorded at that point of the document,
Exactly. My point in this particular discussion is that details of
recording of references to parents don't belong here, even though to
tell the truth I think they don't belong to git _user_ documentation at
all.
> but I personally
> think that the users deserve a chance to learn how a merge is
> recorded in "git merge" documentation.
I doubt a user will gain anything from this sacred knowledge suddenly
being thrown on him when what she is looking for is understanding of
basic merge semantics in GIT.
That said, if you still disagree, please feel free to just drop the
patch.
-- Sergey
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Matthieu Moy @ 2016-10-07 12:47 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Johannes Schindelin, Junio C Hamano, Git Mailing List
In-Reply-To: <CACsJy8ASc7Fxm5XDHFiX9E+bQ8s1MtmEHfc7bZY4C-_GEQr0og@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> On Fri, Oct 7, 2016 at 6:20 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>> Hi Junio,
>>
>> On Thu, 6 Oct 2016, Junio C Hamano wrote:
>>
>>> 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.
>>
>> I guess he is not happy with it because "!!" is quite unintuitive a
>> construct. I know that *I* would have been puzzled by it, asking
>> "What the
>> heck does this do?".
>
> Yep. And I wouldn't want to set a tradition for the next alias type
> '!!!'. There's no good choice to represent a new alias type with a
> leading symbol. This just occurred to me, however, what do you think
> about a new config group for it? With can have something like
> externalAlias.* (or some other name) that lives in parallel with
> alias.*. Then we don't need '!' (or '!!') at all.
Another possibility: !(nocd), which leaves room
for !(keyword1,keyword2,...) if needed later. Also, it is consistent
with the :(word) syntax of pathspecs.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Jakub Narębski @ 2016-10-07 13:31 UTC (permalink / raw)
To: Johannes Schindelin, Junio C Hamano
Cc: Nguyễn Thái Ngọc Duy, git
In-Reply-To: <alpine.DEB.2.20.1610071319520.35196@virtualbox>
Hello, Johannes
W dniu 07.10.2016 o 13:20, Johannes Schindelin pisze:
> On Thu, 6 Oct 2016, Junio C Hamano wrote:
>> 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.
>
> I guess he is not happy with it because "!!" is quite unintuitive a
> construct. I know that *I* would have been puzzled by it, asking "What the
> heck does this do?".
Well, "!" as a prefix is not intuitive either.
Perhaps "!.", because "." is current directory, and the "." command
(that is, alias to "source") doesn't make sense in git aliases.
Note that we would have to teach git completion about new syntax;
or new configuration variable if we go that route.
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Johannes Schindelin @ 2016-10-07 14:11 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <CACsJy8ASc7Fxm5XDHFiX9E+bQ8s1MtmEHfc7bZY4C-_GEQr0og@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2448 bytes --]
Hi Duy,
On Fri, 7 Oct 2016, Duy Nguyen wrote:
> On Fri, Oct 7, 2016 at 6:20 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Thu, 6 Oct 2016, Junio C Hamano wrote:
> >
> >> 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.
> >
> > I guess he is not happy with it because "!!" is quite unintuitive a
> > construct. I know that *I* would have been puzzled by it, asking "What the
> > heck does this do?".
>
> Yep. And I wouldn't want to set a tradition for the next alias type
> '!!!'. There's no good choice to represent a new alias type with a
> leading symbol. This just occurred to me, however, what do you think
> about a new config group for it? With can have something like
> externalAlias.* (or some other name) that lives in parallel with
> alias.*. Then we don't need '!' (or '!!') at all.
But what would the precedence be? externalAlias.xyz wins over alias.xyz?
And we still would need '!' support: tons of people (including myself)
rely on it.
Possibly a better idea would be to use *another* special symbol, one that
makes intuitive sense as a modifier, such as:
[alias]
# This works as before
xyz = !pwd
# As does this
stat = -p status
# This, however, is different:
duy = (nocd)!pwd
This is backwards compatible as "(" is not a part of any Git command, nor
of a valid alias, nor is it commonly used as part of a git-*
executable/script.
It is also kind of a bit more intuitive, I'd wager, and it is also
extensible to future options we may want to introduce.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Johannes Schindelin @ 2016-10-07 14:12 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Duy Nguyen, Junio C Hamano, Git Mailing List
In-Reply-To: <vpqmvig5ns6.fsf@anie.imag.fr>
[-- Attachment #1: Type: text/plain, Size: 2251 bytes --]
Hi Matthieu,
On Fri, 7 Oct 2016, Matthieu Moy wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
> > On Fri, Oct 7, 2016 at 6:20 PM, Johannes Schindelin
> > <Johannes.Schindelin@gmx.de> wrote:
> >> Hi Junio,
> >>
> >> On Thu, 6 Oct 2016, Junio C Hamano wrote:
> >>
> >>> 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.
> >>
> >> I guess he is not happy with it because "!!" is quite unintuitive a
> >> construct. I know that *I* would have been puzzled by it, asking
> >> "What the
> >> heck does this do?".
> >
> > Yep. And I wouldn't want to set a tradition for the next alias type
> > '!!!'. There's no good choice to represent a new alias type with a
> > leading symbol. This just occurred to me, however, what do you think
> > about a new config group for it? With can have something like
> > externalAlias.* (or some other name) that lives in parallel with
> > alias.*. Then we don't need '!' (or '!!') at all.
>
> Another possibility: !(nocd), which leaves room
> for !(keyword1,keyword2,...) if needed later. Also, it is consistent
> with the :(word) syntax of pathspecs.
But is this backwards-compatible? Don't we execute everything that comes
after the exclamation mark as a command-line via shell, where the
parentheses mean "open a subshell"?
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Johannes Schindelin @ 2016-10-07 14:19 UTC (permalink / raw)
To: Jakub Narębski
Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy, git
In-Reply-To: <e1b432bc-97b4-15be-aa44-71921c64cd15@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2014 bytes --]
Hi Kuba,
On Fri, 7 Oct 2016, Jakub Narębski wrote:
> W dniu 07.10.2016 o 13:20, Johannes Schindelin pisze:
> > On Thu, 6 Oct 2016, Junio C Hamano wrote:
> >> 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.
> >
> > I guess he is not happy with it because "!!" is quite unintuitive a
> > construct. I know that *I* would have been puzzled by it, asking "What the
> > heck does this do?".
>
> Well, "!" as a prefix is not intuitive either.
You do not use vi, do you? :-P
In vi, if you enter command mode (typing a colon) and then want to
execute, say, `pwd`, you type !pwd<Enter>
> Perhaps "!.", because "." is current directory, and the "." command
> (that is, alias to "source") doesn't make sense in git aliases.
If you want to execute, say, `pwd` in the current directory, that would
mean you want to write
!.pwd
But that already means "execute `.pwd`"...
> Note that we would have to teach git completion about new syntax;
> or new configuration variable if we go that route.
Why would we? Git's completion does not expand aliases, it only completes
the aliases' names, not their values.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Jeff King @ 2016-10-07 14:20 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Duy Nguyen, Junio C Hamano, Git Mailing List
In-Reply-To: <alpine.DEB.2.20.1610071519450.35196@virtualbox>
On Fri, Oct 07, 2016 at 04:11:34PM +0200, Johannes Schindelin wrote:
> Possibly a better idea would be to use *another* special symbol, one that
> makes intuitive sense as a modifier, such as:
>
> [alias]
> # This works as before
> xyz = !pwd
> # As does this
> stat = -p status
> # This, however, is different:
> duy = (nocd)!pwd
>
> This is backwards compatible as "(" is not a part of any Git command, nor
> of a valid alias, nor is it commonly used as part of a git-*
> executable/script.
>
> It is also kind of a bit more intuitive, I'd wager, and it is also
> extensible to future options we may want to introduce.
I like this much better (like you, I am concerned about things like
"!(foo)" as conflicting with the shell). And I think your "(nocd)!pwd"
example is quite readable.
-Peff
^ permalink raw reply
* Re: [PATCH] clean up confusing suggestion for commit references
From: Jeff King @ 2016-10-07 14:32 UTC (permalink / raw)
To: Heiko Voigt; +Cc: Junio C Hamano, git
In-Reply-To: <20161007095638.GA55445@book.hvoigt.net>
On Fri, Oct 07, 2016 at 11:56:38AM +0200, Heiko Voigt wrote:
> The description for referencing commits looks as if it is contradicting
> the example, since it is itself enclosed in double quotes. Lets use
> single quotes around the description and include the double quotes in
> the description so it matches the example.
> ---
> Sorry for opening this up again but I just looked up the format and was
> like: "Umm, which one is now the correct one..."
>
> For this makes more sense. What do others think?
Looking over the threads, I wasn't sure there was consensus[1,2]. So it would
be equally correct to drop the quotes from the example.
I dunno. I am in favor of no-quotes, myself, so maybe I am just
manufacturing dissent in my mind. :)
-Peff
[1] http://public-inbox.org/git/a9731f60-5c30-0bc6-f73a-f7ffb7bd4231@kdbg.org/
[2] http://public-inbox.org/git/20160829183015.2uqnfezekjfa3ott@sigill.intra.peff.net/
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Matthieu Moy @ 2016-10-07 14:31 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Duy Nguyen, Junio C Hamano, Git Mailing List
In-Reply-To: <alpine.DEB.2.20.1610071611550.35196@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi Matthieu,
>
> On Fri, 7 Oct 2016, Matthieu Moy wrote:
>
>> Another possibility: !(nocd), which leaves room
>> for !(keyword1,keyword2,...) if needed later. Also, it is consistent
>> with the :(word) syntax of pathspecs.
>
> But is this backwards-compatible? Don't we execute everything that comes
> after the exclamation mark as a command-line via shell, where the
> parentheses mean "open a subshell"?
Good point. I can imagine someone already having
[alias]
foo = !(cd bar && $something) && $something_else
Your proposed (keyword)! is better.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* [PATCH v2 1/3] serialize collection of changed submodules
From: Heiko Voigt @ 2016-10-07 15:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: Heiko Voigt, Jeff King, Stefan Beller, git, Jens.Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <cover.1475851621.git.hvoigt@hvoigt.net>
To check whether a submodule needs to be pushed we need to collect all
changed submodules. Lets collect them first and then execute the
possibly expensive test whether certain revisions are already pushed
only once per submodule.
There is further potential for optimization since we can assemble one
command and only issued that instead of one call for each remote ref in
the submodule.
Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
submodule.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 58 insertions(+), 5 deletions(-)
diff --git a/submodule.c b/submodule.c
index 2de06a3351..59c9d15905 100644
--- a/submodule.c
+++ b/submodule.c
@@ -554,19 +554,34 @@ static int submodule_needs_pushing(const char *path, const unsigned char sha1[20
return 0;
}
+static struct sha1_array *get_sha1s_from_list(struct string_list *submodules,
+ const char *path)
+{
+ struct string_list_item *item;
+
+ item = string_list_insert(submodules, path);
+ if (item->util)
+ return (struct sha1_array *) item->util;
+
+ /* NEEDSWORK: should we have sha1_array_init()? */
+ item->util = xcalloc(1, sizeof(struct sha1_array));
+ return (struct sha1_array *) item->util;
+}
+
static void collect_submodules_from_diff(struct diff_queue_struct *q,
struct diff_options *options,
void *data)
{
int i;
- struct string_list *needs_pushing = data;
+ struct string_list *submodules = data;
for (i = 0; i < q->nr; i++) {
struct diff_filepair *p = q->queue[i];
+ struct sha1_array *hashes;
if (!S_ISGITLINK(p->two->mode))
continue;
- if (submodule_needs_pushing(p->two->path, p->two->oid.hash))
- string_list_insert(needs_pushing, p->two->path);
+ hashes = get_sha1s_from_list(submodules, p->two->path);
+ sha1_array_append(hashes, p->two->oid.hash);
}
}
@@ -582,14 +597,41 @@ static void find_unpushed_submodule_commits(struct commit *commit,
diff_tree_combined_merge(commit, 1, &rev);
}
+struct collect_submodule_from_sha1s_data {
+ char *submodule_path;
+ struct string_list *needs_pushing;
+};
+
+static void collect_submodules_from_sha1s(const unsigned char sha1[20],
+ void *data)
+{
+ struct collect_submodule_from_sha1s_data *me =
+ (struct collect_submodule_from_sha1s_data *) data;
+
+ if (submodule_needs_pushing(me->submodule_path, sha1))
+ string_list_insert(me->needs_pushing, me->submodule_path);
+}
+
+static void free_submodules_sha1s(struct string_list *submodules)
+{
+ int i;
+ for (i = 0; i < submodules->nr; i++) {
+ struct string_list_item *item = &submodules->items[i];
+ struct sha1_array *hashes = (struct sha1_array *) item->util;
+ sha1_array_clear(hashes);
+ }
+ string_list_clear(submodules, 1);
+}
+
int find_unpushed_submodules(unsigned char new_sha1[20],
const char *remotes_name, struct string_list *needs_pushing)
{
struct rev_info rev;
struct commit *commit;
const char *argv[] = {NULL, NULL, "--not", "NULL", NULL};
- int argc = ARRAY_SIZE(argv) - 1;
+ int argc = ARRAY_SIZE(argv) - 1, i;
char *sha1_copy;
+ struct string_list submodules = STRING_LIST_INIT_DUP;
struct strbuf remotes_arg = STRBUF_INIT;
@@ -603,12 +645,23 @@ int find_unpushed_submodules(unsigned char new_sha1[20],
die("revision walk setup failed");
while ((commit = get_revision(&rev)) != NULL)
- find_unpushed_submodule_commits(commit, needs_pushing);
+ find_unpushed_submodule_commits(commit, &submodules);
reset_revision_walk();
free(sha1_copy);
strbuf_release(&remotes_arg);
+ for (i = 0; i < submodules.nr; i++) {
+ struct string_list_item *item = &submodules.items[i];
+ struct collect_submodule_from_sha1s_data data;
+ data.submodule_path = item->string;
+ data.needs_pushing = needs_pushing;
+ sha1_array_for_each_unique((struct sha1_array *) item->util,
+ collect_submodules_from_sha1s,
+ &data);
+ }
+ free_submodules_sha1s(&submodules);
+
return needs_pushing->nr;
}
--
2.10.1.637.g09b28c5
^ permalink raw reply related
* [PATCH v2 2/3] serialize collection of refs that contain submodule changes
From: Heiko Voigt @ 2016-10-07 15:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: Heiko Voigt, Jeff King, Stefan Beller, git, Jens.Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <cover.1475851621.git.hvoigt@hvoigt.net>
We are iterating over each pushed ref and want to check whether it
contains changes to submodules. Instead of immediately checking each ref
lets first collect them and then do the check for all of them in one
revision walk.
Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
submodule.c | 36 +++++++++++++++++++++---------------
submodule.h | 5 +++--
transport.c | 29 +++++++++++++++++++++--------
3 files changed, 45 insertions(+), 25 deletions(-)
diff --git a/submodule.c b/submodule.c
index 59c9d15905..5044afc2f8 100644
--- a/submodule.c
+++ b/submodule.c
@@ -522,6 +522,13 @@ static int has_remote(const char *refname, const struct object_id *oid,
return 1;
}
+static int append_hash_to_argv(const unsigned char sha1[20], void *data)
+{
+ struct argv_array *argv = (struct argv_array *) data;
+ argv_array_push(argv, sha1_to_hex(sha1));
+ return 0;
+}
+
static int submodule_needs_pushing(const char *path, const unsigned char sha1[20])
{
if (add_submodule_odb(path) || !lookup_commit_reference(sha1))
@@ -623,24 +630,24 @@ static void free_submodules_sha1s(struct string_list *submodules)
string_list_clear(submodules, 1);
}
-int find_unpushed_submodules(unsigned char new_sha1[20],
+int find_unpushed_submodules(struct sha1_array *hashes,
const char *remotes_name, struct string_list *needs_pushing)
{
struct rev_info rev;
struct commit *commit;
- const char *argv[] = {NULL, NULL, "--not", "NULL", NULL};
- int argc = ARRAY_SIZE(argv) - 1, i;
- char *sha1_copy;
+ int i;
struct string_list submodules = STRING_LIST_INIT_DUP;
+ struct argv_array argv = ARGV_ARRAY_INIT;
- struct strbuf remotes_arg = STRBUF_INIT;
-
- strbuf_addf(&remotes_arg, "--remotes=%s", remotes_name);
init_revisions(&rev, NULL);
- sha1_copy = xstrdup(sha1_to_hex(new_sha1));
- argv[1] = sha1_copy;
- argv[3] = remotes_arg.buf;
- setup_revisions(argc, argv, &rev, NULL);
+
+ /* argv.argv[0] will be ignored by setup_revisions */
+ argv_array_push(&argv, "find_unpushed_submodules");
+ sha1_array_for_each_unique(hashes, append_hash_to_argv, &argv);
+ argv_array_push(&argv, "--not");
+ argv_array_pushf(&argv, "--remotes=%s", remotes_name);
+
+ setup_revisions(argv.argc, argv.argv, &rev, NULL);
if (prepare_revision_walk(&rev))
die("revision walk setup failed");
@@ -648,8 +655,7 @@ int find_unpushed_submodules(unsigned char new_sha1[20],
find_unpushed_submodule_commits(commit, &submodules);
reset_revision_walk();
- free(sha1_copy);
- strbuf_release(&remotes_arg);
+ argv_array_clear(&argv);
for (i = 0; i < submodules.nr; i++) {
struct string_list_item *item = &submodules.items[i];
@@ -687,12 +693,12 @@ static int push_submodule(const char *path)
return 1;
}
-int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name)
+int push_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name)
{
int i, ret = 1;
struct string_list needs_pushing = STRING_LIST_INIT_DUP;
- if (!find_unpushed_submodules(new_sha1, remotes_name, &needs_pushing))
+ if (!find_unpushed_submodules(hashes, remotes_name, &needs_pushing))
return 1;
for (i = 0; i < needs_pushing.nr; i++) {
diff --git a/submodule.h b/submodule.h
index d9e197a948..065b2f0a2a 100644
--- a/submodule.h
+++ b/submodule.h
@@ -3,6 +3,7 @@
struct diff_options;
struct argv_array;
+struct sha1_array;
enum {
RECURSE_SUBMODULES_CHECK = -4,
@@ -62,9 +63,9 @@ int submodule_uses_gitfile(const char *path);
int ok_to_remove_submodule(const char *path);
int merge_submodule(unsigned char result[20], const char *path, const unsigned char base[20],
const unsigned char a[20], const unsigned char b[20], int search);
-int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name,
+int find_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name,
struct string_list *needs_pushing);
-int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);
+int push_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name);
void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
int parallel_submodules(void);
diff --git a/transport.c b/transport.c
index 94d6dc3725..05f2ce83f1 100644
--- a/transport.c
+++ b/transport.c
@@ -903,23 +903,36 @@ int transport_push(struct transport *transport,
if ((flags & TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND) && !is_bare_repository()) {
struct ref *ref = remote_refs;
+ struct sha1_array hashes = SHA1_ARRAY_INIT;
+
for (; ref; ref = ref->next)
- if (!is_null_oid(&ref->new_oid) &&
- !push_unpushed_submodules(ref->new_oid.hash,
- transport->remote->name))
- die ("Failed to push all needed submodules!");
+ if (!is_null_oid(&ref->new_oid))
+ sha1_array_append(&hashes, ref->new_oid.hash);
+
+ if (!push_unpushed_submodules(&hashes, transport->remote->name)) {
+ sha1_array_clear(&hashes);
+ die ("Failed to push all needed submodules!");
+ }
+ sha1_array_clear(&hashes);
}
if ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
TRANSPORT_RECURSE_SUBMODULES_CHECK)) && !is_bare_repository()) {
struct ref *ref = remote_refs;
struct string_list needs_pushing = STRING_LIST_INIT_DUP;
+ struct sha1_array hashes = SHA1_ARRAY_INIT;
for (; ref; ref = ref->next)
- if (!is_null_oid(&ref->new_oid) &&
- find_unpushed_submodules(ref->new_oid.hash,
- transport->remote->name, &needs_pushing))
- die_with_unpushed_submodules(&needs_pushing);
+ if (!is_null_oid(&ref->new_oid))
+ sha1_array_append(&hashes, ref->new_oid.hash);
+
+ if (find_unpushed_submodules(&hashes, transport->remote->name,
+ &needs_pushing)) {
+ sha1_array_clear(&hashes);
+ die_with_unpushed_submodules(&needs_pushing);
+ }
+ string_list_clear(&needs_pushing, 0);
+ sha1_array_clear(&hashes);
}
push_ret = transport->push_refs(transport, remote_refs, flags);
--
2.10.1.637.g09b28c5
^ permalink raw reply related
* [PATCH v2 0/3] Speedup finding of unpushed submodules
From: Heiko Voigt @ 2016-10-07 15:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: Heiko Voigt, Jeff King, Stefan Beller, git, Jens.Lehmann,
Fredrik Gustafsson, Leandro Lucarella
You can find the first iteration of this series as part of this thread:
http://public-inbox.org/git/%3C20160914173124.GA7613@sandbox%3E/
All mentioned issues should be fixed. I dropped the last patch which was
the cause of the broken tests.
This should optimize every part of this test to a nice speed if you are
pushing to a remote. The only case that is still broken/slow as hell is
when calling push with a direct url.
I am thinking whether we should maybe error out with a "not implemented"
message or something and mention that --recurse-submoules does not work
with direct urls? But we might want to have another look at performance
with this patch included. Maybe it is actually useable with the last
patch included which was not yet on pu.
Cheers Heiko
Heiko Voigt (3):
serialize collection of changed submodules
serialize collection of refs that contain submodule changes
batch check whether submodule needs pushing into one call
submodule.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++--------------
submodule.h | 5 +--
transport.c | 29 ++++++++++-----
3 files changed, 114 insertions(+), 36 deletions(-)
--
2.10.1.637.g09b28c5
^ permalink raw reply
* [PATCH v2 3/3] batch check whether submodule needs pushing into one call
From: Heiko Voigt @ 2016-10-07 15:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: Heiko Voigt, Jeff King, Stefan Beller, git, Jens.Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <cover.1475851621.git.hvoigt@hvoigt.net>
We run a command for each sha1 change in a submodule. This is
unnecessary since we can simply batch all sha1's we want to check into
one command. Lets do it so we can speedup the check when many submodule
changes are in need of checking.
Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
submodule.c | 63 +++++++++++++++++++++++++++++++++----------------------------
1 file changed, 34 insertions(+), 29 deletions(-)
diff --git a/submodule.c b/submodule.c
index 5044afc2f8..a05c2a34b1 100644
--- a/submodule.c
+++ b/submodule.c
@@ -529,27 +529,49 @@ static int append_hash_to_argv(const unsigned char sha1[20], void *data)
return 0;
}
-static int submodule_needs_pushing(const char *path, const unsigned char sha1[20])
+static int check_has_hash(const unsigned char sha1[20], void *data)
{
- if (add_submodule_odb(path) || !lookup_commit_reference(sha1))
+ int *has_hash = (int *) data;
+
+ if (!lookup_commit_reference(sha1))
+ *has_hash = 0;
+
+ return 0;
+}
+
+static int submodule_has_hashes(const char *path, struct sha1_array *hashes)
+{
+ int has_hash = 1;
+
+ if (add_submodule_odb(path))
+ return 0;
+
+ sha1_array_for_each_unique(hashes, check_has_hash, &has_hash);
+ return has_hash;
+}
+
+static int submodule_needs_pushing(const char *path, struct sha1_array *hashes)
+{
+ if (!submodule_has_hashes(path, hashes))
return 0;
if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {
struct child_process cp = CHILD_PROCESS_INIT;
- const char *argv[] = {"rev-list", NULL, "--not", "--remotes", "-n", "1" , NULL};
struct strbuf buf = STRBUF_INIT;
int needs_pushing = 0;
- argv[1] = sha1_to_hex(sha1);
- cp.argv = argv;
+ argv_array_push(&cp.args, "rev-list");
+ sha1_array_for_each_unique(hashes, append_hash_to_argv, &cp.args);
+ argv_array_pushl(&cp.args, "--not", "--remotes", "-n", "1" , NULL);
+
prepare_submodule_repo_env(&cp.env_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
cp.out = -1;
cp.dir = path;
if (start_command(&cp))
- die("Could not run 'git rev-list %s --not --remotes -n 1' command in submodule %s",
- sha1_to_hex(sha1), path);
+ die("Could not run 'git rev-list <hashes> --not --remotes -n 1' command in submodule %s",
+ path);
if (strbuf_read(&buf, cp.out, 41))
needs_pushing = 1;
finish_command(&cp);
@@ -604,21 +626,6 @@ static void find_unpushed_submodule_commits(struct commit *commit,
diff_tree_combined_merge(commit, 1, &rev);
}
-struct collect_submodule_from_sha1s_data {
- char *submodule_path;
- struct string_list *needs_pushing;
-};
-
-static void collect_submodules_from_sha1s(const unsigned char sha1[20],
- void *data)
-{
- struct collect_submodule_from_sha1s_data *me =
- (struct collect_submodule_from_sha1s_data *) data;
-
- if (submodule_needs_pushing(me->submodule_path, sha1))
- string_list_insert(me->needs_pushing, me->submodule_path);
-}
-
static void free_submodules_sha1s(struct string_list *submodules)
{
int i;
@@ -658,13 +665,11 @@ int find_unpushed_submodules(struct sha1_array *hashes,
argv_array_clear(&argv);
for (i = 0; i < submodules.nr; i++) {
- struct string_list_item *item = &submodules.items[i];
- struct collect_submodule_from_sha1s_data data;
- data.submodule_path = item->string;
- data.needs_pushing = needs_pushing;
- sha1_array_for_each_unique((struct sha1_array *) item->util,
- collect_submodules_from_sha1s,
- &data);
+ struct string_list_item *submodule = &submodules.items[i];
+ struct sha1_array *hashes = (struct sha1_array *) submodule->util;
+
+ if (submodule_needs_pushing(submodule->string, hashes))
+ string_list_insert(needs_pushing, submodule->string);
}
free_submodules_sha1s(&submodules);
--
2.10.1.637.g09b28c5
^ permalink raw reply related
* Re: Systems with old regex system headers/libraries don't pick up git's compat/regex header file
From: Richard Lloyd @ 2016-10-07 15:45 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20161006191127.2vjtmxl7ygjeqcbk@sigill.intra.peff.net>
On 06/10/16 20:11, Jeff King wrote:
> 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.
This doesn't work because the check in git-compat-util.h only looks
for REG_STARTEND being defined (if it isn't, it #error's out).
That define is not mentioned anywhere else other than in the
compat/regex tree, which is why I used -Icompat/regex to pick up
<regex.h> from there - this was the "easiest" solution for me on
HP-UX 11.
Note that with this inclusion change, the source compiled and linked
fine on HP-UX 11 and git passed its tests, including the regex-based ones.
Richard K. Lloyd, E-mail: richard.lloyd@connectinternetsolutions.com
Connect Internet Solutions, WWW: https://www.connectinternetsolutions.com/
4th Floor, New Barratt House,
47, North John Street,
Liverpool,
Merseyside, UK. L2 6SG
>
> -Peff
>
--
This e-mail (and any attachments) is private and confidential. If you have
received it in error, please notify the sender immediately and delete it
from your system. Do not use, copy or disclose the information in any way
nor act in reliance on it.
Any views expressed in this message are those of the individual sender,
except where the sender specifically states them to be the views of Connect
Internet Solutions Ltd. This e-mail and any attachments are believed to be
virus free but it is the recipient's responsibility to ensure that they are.
Connect Internet Solutions Ltd
(A company registered in England No: 04424350)
Registered Office: 4th Floor, New Barratt House, 47 North John Street,
Liverpool, L2 6SG
Telephone: +44 (0) 151 282 4321
VAT registration number: 758 2838 85
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Jakub Narębski @ 2016-10-07 15:55 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy, git
In-Reply-To: <alpine.DEB.2.20.1610071616390.35196@virtualbox>
W dniu 07.10.2016 o 16:19, Johannes Schindelin pisze:
> On Fri, 7 Oct 2016, Jakub Narębski wrote:
>> Note that we would have to teach git completion about new syntax;
>> or new configuration variable if we go that route.
>
> Why would we? Git's completion does not expand aliases, it only completes
> the aliases' names, not their values.
Because Git completion finds out which _options_ and _arguments_
to complete for an alias based on its expansion.
Yes, this is nice bit of trickery...
--
Jakub Narębski
^ 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-07 16:06 UTC (permalink / raw)
To: Richard Lloyd; +Cc: git
In-Reply-To: <4ac0ce84-ca6c-3650-ef5e-e13c54c60504@connectinternetsolutions.com>
On Fri, Oct 07, 2016 at 04:45:08PM +0100, Richard Lloyd wrote:
> On 06/10/16 20:11, Jeff King wrote:
> > 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.
>
> This doesn't work because the check in git-compat-util.h only looks
> for REG_STARTEND being defined (if it isn't, it #error's out).
>
> That define is not mentioned anywhere else other than in the
> compat/regex tree, which is why I used -Icompat/regex to pick up
> <regex.h> from there - this was the "easiest" solution for me on
> HP-UX 11.
I'm confused. Setting NO_REGEX in the Makefile will add -Icompat/regex
to your compiler invocation. So git-compat-util.h should pick up our
compat regex routines, which _do_ have REG_STARTEND.
How are you building? Doing:
make NO_REGEX=1
is supposed to work, and if it doesn't, there's a bug.
-Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox