* Re: [PATCH] Add more proposals to SoC 2017 ideas page. (lots of submodule stuff)
From: Jeff King @ 2017-01-25 22:06 UTC (permalink / raw)
To: Stefan Beller; +Cc: git
In-Reply-To: <20170125215931.26339-1-sbeller@google.com>
On Wed, Jan 25, 2017 at 01:59:31PM -0800, Stefan Beller wrote:
> This applies to the repo at https://github.com/git/git.github.io
Thanks. I've applied and pushed, though I'll admit I didn't really read
it carefully for content. A few of the ideas look like they would need
to be aggregated to be big enough for a SoC project, but that can be
fleshed out in future patches (i.e., I don't think we care enough about
history to have people polish and re-roll what are essentially wiki
edits).
If you (or anybody interested in working on this) would prefer direct
push access to the git.github.io repo, I'm happy to set that up.
> If you're looking for a co-admin/mentors, I can help out.
I may take you up on the co-admin thing. I think it's good to have a
backup, just in case.
Anything you put on the Ideas page, you should probably be willing to
mentor. We definitely _don't_ want Ideas that don't have a mentor.
I think in general that each idea should have the possible mentors
listed below it.
-Peff
^ permalink raw reply
* Re: [PATCH] Add more proposals to SoC 2017 ideas page. (lots of submodule stuff)
From: Stefan Beller @ 2017-01-25 22:11 UTC (permalink / raw)
To: Jeff King; +Cc: git@vger.kernel.org
In-Reply-To: <20170125220632.whjnpvrnhve2h6f6@sigill.intra.peff.net>
On Wed, Jan 25, 2017 at 2:06 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Jan 25, 2017 at 01:59:31PM -0800, Stefan Beller wrote:
>
>> This applies to the repo at https://github.com/git/git.github.io
>
> Thanks. I've applied and pushed, though I'll admit I didn't really read
> it carefully for content. A few of the ideas look like they would need
> to be aggregated to be big enough for a SoC project, but that can be
> fleshed out in future patches (i.e., I don't think we care enough about
> history to have people polish and re-roll what are essentially wiki
> edits).
Yeah, I wasn't sure if the ideas were meant to also contain microprojects
so I wrote down everything that came to mind, that I do not intend to work on
myself over the next couple month.
>
> If you (or anybody interested in working on this) would prefer direct
> push access to the git.github.io repo, I'm happy to set that up.
Yeah I wouldn't mind direct push access there, then I could fixup
what I just sent you, e.g. adding myself as a possible mentor or
re-shuffling these ideas. :)
>
>> If you're looking for a co-admin/mentors, I can help out.
>
> I may take you up on the co-admin thing. I think it's good to have a
> backup, just in case.
>
> Anything you put on the Ideas page, you should probably be willing to
> mentor. We definitely _don't_ want Ideas that don't have a mentor.
agreed.
> I think in general that each idea should have the possible mentors
> listed below it.
ok, I can make a patch for that; but pushing seems (slightly) easier than
mailing a patch.
Thanks,
Stefan
^ permalink raw reply
* Force Confirmation for Dropping Changed Lines
From: Hilco Wijbenga @ 2017-01-25 22:16 UTC (permalink / raw)
To: Git Users
Hi all,
Most of the time, when a later commit changes a line in an identical
fashion during, say, a rebase, you want Git to silently continue by
dropping the duplicate change from the later commit. I have a common
(for me) scenario where I want Git to specifically ask me to resolve
this "conflict" myself instead of simply assuming that the change has
already been applied.
Let's say I have a file my-code.x that starts with a line indicating
its version:
===== my-code.x =====
VERSION=1.2
line 1
line 2
line 3
=====
In my branch my-branch off of master, I make a change:
===== my-code.x =====
VERSION=1.3
line 1
line 2
line 2a
line 3
=====
Now someone else makes a different change on master and pushes it ([1]):
===== my-code.x =====
VERSION=1.3
line 1
line 2
line 3
line 4
=====
When I rebase my-branch onto origin/master, I get no conflicts and
everything seems fine ([2]):
===== my-code.x =====
VERSION=1.3
line 1
line 2
line 2a
line 3
line 4
=====
Except that I should have used VERSION=1.4, not VERSION=1.3 because I
made a change to my-code.x. Obviously, for a single file this is easy
to remember/check but when it's hidden among lots of files and commits
it becomes very hard to find these types of errors.
How can I force Git to not assume my change to the first line is "redundant"?
Cheers,
Hilco
[1] Note that this someone is (correctly) using the same, new version of 1.3.
[2] If my example is actually incorrect, then please just pretend
there are no conflicts.
^ permalink raw reply
* Re: [PATCH v1 1/3] blame: add --aggregate option
From: Jeff King @ 2017-01-25 22:22 UTC (permalink / raw)
To: Edmundo Carmona Antoranz; +Cc: git, apelisse, kevin, gitster
In-Reply-To: <20170125052734.17550-1-eantoranz@gmail.com>
On Tue, Jan 24, 2017 at 11:27:32PM -0600, Edmundo Carmona Antoranz wrote:
> To avoid taking up so much space on normal output printing duplicate
> information on consecutive lines that "belong" to the same revision,
> this option allows to print a single line with the information about
> the revision before the lines of content themselves.
I admit I have not followed the preceding discussion closely, so ignore
me if my suggestion is way off base.
But it really seems like the various outputs you are looking for are
really all about customizing git-blame's human-readable output.
Would it be more productive to move towards a "--format" option that
shows custom items for each line? It could build on the custom formats
in the pretty-print code (though I think you would want to add some
custom ones, like filename, line number, line content, etc).
I know that only does half of what you want, which is making some output
not just per-line, but per-block. But that can come easily on top, if we
allow separate per-line and per-block formats (which would default to
the current output and the empty string, respectively).
Then you could do something like:
git blame --format-block='%h %an <%ae>%n %s' \
--format-line='\t%(filename):%(linenumber) %(linecontent)'
and get something like:
1234abcd A U Thor <author@example.com>
initial commit
foo.c:1 #include <foo.h>
foo.c:2 #include <bar.h>
5678abcd A U Thor <author@example.com>
add third include
foo.c:3 #include <third.h>
and so on. But people can mix and match what they want to see on each
line, and what they'd rather push to other lines.
I don't have a huge interest in the feature myself. I switched to "tig
blame" years ago and never looked back. But it just seems like your
options touch no this kind of flexibility, but will limit somebody as
soon as they want to switch around which information goes where.
-Peff
^ permalink raw reply
* Re: Force Confirmation for Dropping Changed Lines
From: Jacob Keller @ 2017-01-25 22:24 UTC (permalink / raw)
To: Hilco Wijbenga; +Cc: Git Users
In-Reply-To: <CAE1pOi2YZayEfKxxh3gsTds1mQ9L1E9AW=wPnmW=Dg=-EMj=tw@mail.gmail.com>
On Wed, Jan 25, 2017 at 2:16 PM, Hilco Wijbenga
<hilco.wijbenga@gmail.com> wrote:
> How can I force Git to not assume my change to the first line is "redundant"?
>
My guess is that you probably want a custom merge driver for your file
types. That's where I would look initially.
Thanks,
Jake
> Cheers,
> Hilco
>
> [1] Note that this someone is (correctly) using the same, new version of 1.3.
> [2] If my example is actually incorrect, then please just pretend
> there are no conflicts.
^ permalink raw reply
* Re: [PATCH] Add more proposals to SoC 2017 ideas page. (lots of submodule stuff)
From: Jeff King @ 2017-01-25 22:26 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org
In-Reply-To: <CAGZ79kb_g5Wq=Aeo1RH-iA5M-drU5Gm1LAJZuPZU7oe=xdHaOQ@mail.gmail.com>
On Wed, Jan 25, 2017 at 02:11:15PM -0800, Stefan Beller wrote:
> > Thanks. I've applied and pushed, though I'll admit I didn't really read
> > it carefully for content. A few of the ideas look like they would need
> > to be aggregated to be big enough for a SoC project, but that can be
> > fleshed out in future patches (i.e., I don't think we care enough about
> > history to have people polish and re-roll what are essentially wiki
> > edits).
>
> Yeah, I wasn't sure if the ideas were meant to also contain microprojects
> so I wrote down everything that came to mind, that I do not intend to work on
> myself over the next couple month.
Microprojects go on their own page. But make sure that they really are
"micro" sized for an applicant.
> > If you (or anybody interested in working on this) would prefer direct
> > push access to the git.github.io repo, I'm happy to set that up.
>
> Yeah I wouldn't mind direct push access there, then I could fixup
> what I just sent you, e.g. adding myself as a possible mentor or
> re-shuffling these ideas. :)
OK, done. For anybody else interested, I just need to know your GitHub
username.
-Peff
^ permalink raw reply
* Re: [PATCH] Add more proposals to SoC 2017 ideas page. (lots of submodule stuff)
From: Stefan Beller @ 2017-01-25 22:34 UTC (permalink / raw)
To: Jeff King; +Cc: git@vger.kernel.org
In-Reply-To: <20170125222627.jlswvwmzli62fnnt@sigill.intra.peff.net>
On Wed, Jan 25, 2017 at 2:26 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Jan 25, 2017 at 02:11:15PM -0800, Stefan Beller wrote:
>
>> > Thanks. I've applied and pushed, though I'll admit I didn't really read
>> > it carefully for content. A few of the ideas look like they would need
>> > to be aggregated to be big enough for a SoC project, but that can be
>> > fleshed out in future patches (i.e., I don't think we care enough about
>> > history to have people polish and re-roll what are essentially wiki
>> > edits).
>>
>> Yeah, I wasn't sure if the ideas were meant to also contain microprojects
>> so I wrote down everything that came to mind, that I do not intend to work on
>> myself over the next couple month.
>
> Microprojects go on their own page. But make sure that they really are
> "micro" sized for an applicant.
Yeah micro as in real micro.
e.g. fix the SubmittingPatches doc, after confusion about "signing",
start reading here
https://public-inbox.org/git/923cd4e4-5c9c-4eaf-0fea-6deff6875b88@tngtech.com/
(I did not push it, as I'd need to copy over the micro projects page from 2016,
and then find out where to put links to have the web page not look broken)
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND
From: Junio C Hamano @ 2017-01-25 22:35 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Segev Finer
In-Reply-To: <alpine.DEB.2.20.1701251327090.3469@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Now, with the patch in question (without the follow-up, which I would like
> to ask you to ignore, just like you did so far), Git would not figure out
> that your script calls PuTTY eventually. The work-around? Easy:
>
> DUMMY=/plink.exe /path/to/junio-is-a-superstar.sh
Think about how you would explain that to an end-user in our
document? You'll need to explain how exactly the auto-detection
works, so that the user can "exploit" the loophole to do that. And
what maintenance burden does it add when auto-detection is updated?
I think I know you well enough that you know well enough that it is
too ugly to live, and I suspect that the above is a tongue-in-cheek
"arguing for the sake of argument" and would not need a serious
response, but just in case...
> ...
> Of course, given our discussion I think all of this should be documented
> in the commit message as well as in the man page.
Yes. Here is what comes on an obvious clean-up patch (which will be
sent as a follow-up to this message).
-- >8 --
Subject: [PATCH] connect: core.sshvariant to correct misidentification
While the logic we have been using to guess which variant of SSH
implementation is in use by looking at the name of the program has
been robust enough for GIT_SSH (which does not go through the shell,
hence it can only spell the name the SSH program and nothing else),
extending it to GIT_SSH_COMMAND and core.sshcommand opens it for
larger chance of misidentification, because these can specify
arbitrary shell command, and a simple "the first word on the line
must be the command name" heuristic may not even look at the command
name at all.
As any effort to improve the heuristic will give us only diminishing
returns, and especially given that most of the users are expected to
have a command line for which the heuristic would work correctly,
give an explicit escape hatch to override a misidentification, just
in case one happens.
It is arguably bad to add this new variable to the core.* set, in
the sense that you'll never see this variable if you do not interact
with other repositories over ssh, but core.sshcommand is already
there for some reason, so let's match it.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/config.txt | 13 +++++++++++++
connect.c | 26 ++++++++++++++++++++++++++
2 files changed, 39 insertions(+)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index af2ae4cc02..8ea1db49c6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -455,6 +455,19 @@ core.sshCommand::
the `GIT_SSH_COMMAND` environment variable and is overridden
when the environment variable is set.
+core.sshVariant::
+ SSH implementations used by Git when running `git fetch`,
+ `git clone`, and `git push` use slightly different command
+ line options (e.g. PuTTY and TortoisePlink use `-P <port>`
+ while OpenSSH uses `-p <port>` to specify the port number.
+ TortoisePlink in addition requires `-batch` option to be
+ passed). Git guesses which variant is in use correctly from
+ the name of the ssh command used (see `core.sshCommand`
+ configuration variable and also `GIT_SSH_COMMAND`
+ environment variable) most of the time. You can set this
+ variable to 'putty', 'tortoiseplink', or 'ssh' to correct it
+ when Git makes an incorrect guess.
+
core.ignoreStat::
If true, Git will avoid using lstat() calls to detect if files have
changed by setting the "assume-unchanged" bit for those tracked files
diff --git a/connect.c b/connect.c
index aa51b33bfc..358e9c06f0 100644
--- a/connect.c
+++ b/connect.c
@@ -691,6 +691,29 @@ static const char *get_ssh_command(void)
return NULL;
}
+static void override_ssh_variant(int *port_option, int *needs_batch)
+{
+ const char *variant;
+
+ if (git_config_get_string_const("core.sshvariant", &variant))
+ return;
+ if (!strcmp(variant, "tortoiseplink")) {
+ *port_option = 'P';
+ *needs_batch = 1;
+ } else if (!strcmp(variant, "putty")) {
+ *port_option = 'P';
+ *needs_batch = 0;
+ } else {
+ /* default */
+ if (strcmp(variant, "ssh")) {
+ warning(_("core.sshvariant: unknown value '%s'"), variant);
+ warning(_("using OpenSSH compatible behaviour"));
+ }
+ *port_option = 'p';
+ *needs_batch = 0;
+ }
+}
+
/*
* This returns a dummy child_process if the transport protocol does not
* need fork(2), or a struct child_process object if it does. Once done,
@@ -836,6 +859,9 @@ struct child_process *git_connect(int fd[2], const char *url,
argv_array_push(&conn->args, "-4");
else if (flags & CONNECT_IPV6)
argv_array_push(&conn->args, "-6");
+
+ override_ssh_variant(&port_option, &needs_batch);
+
if (needs_batch)
argv_array_push(&conn->args, "-batch");
if (port) {
--
2.11.0-699-ga1f1475476
^ permalink raw reply related
* Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND
From: Junio C Hamano @ 2017-01-25 22:37 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Segev Finer
In-Reply-To: <xmqqinp2939j.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Yes. Here is what comes on an obvious clean-up patch (which will be
> sent as a follow-up to this message).
... and this is the "obvious clean-up patch" to apply directly on
top of Segev's GIT_SSH_COMMAND support, which comes before the
previous one.
-- >8 --
Subject: [PATCH] connect: rename tortoiseplink and putty variables
One of these two may have originally been named after "what exact
SSH implementation do we have" so that we can tweak the command line
options, but these days "putty=1" no longer means "We are using the
plink SSH implementation that comes with PuTTY". It is set when we
guess that either PuTTY plink or Tortoiseplink is in use.
Rename them after what effect is desired. The current "putty"
option is about using "-P <port>" when OpenSSH would use "-p <port>",
so rename it to port_option whose value is either 'p' or 'P". The
other one is about passing an extra command line option "-batch",
so rename it needs_batch.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
connect.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/connect.c b/connect.c
index c81f77001b..aa51b33bfc 100644
--- a/connect.c
+++ b/connect.c
@@ -769,7 +769,8 @@ struct child_process *git_connect(int fd[2], const char *url,
conn->in = conn->out = -1;
if (protocol == PROTO_SSH) {
const char *ssh;
- int putty = 0, tortoiseplink = 0;
+ int needs_batch = 0;
+ int port_option = 'p';
char *ssh_host = hostandport;
const char *port = NULL;
char *ssh_argv0 = NULL;
@@ -819,12 +820,14 @@ struct child_process *git_connect(int fd[2], const char *url,
if (ssh_argv0) {
const char *base = basename(ssh_argv0);
- tortoiseplink = !strcasecmp(base, "tortoiseplink") ||
- !strcasecmp(base, "tortoiseplink.exe");
- putty = tortoiseplink ||
- !strcasecmp(base, "plink") ||
- !strcasecmp(base, "plink.exe");
-
+ if (!strcasecmp(base, "tortoiseplink") ||
+ !strcasecmp(base, "tortoiseplink.exe")) {
+ port_option = 'P';
+ needs_batch = 1;
+ } else if (!strcasecmp(base, "plink") ||
+ !strcasecmp(base, "plink.exe")) {
+ port_option = 'P';
+ }
free(ssh_argv0);
}
@@ -833,11 +836,10 @@ struct child_process *git_connect(int fd[2], const char *url,
argv_array_push(&conn->args, "-4");
else if (flags & CONNECT_IPV6)
argv_array_push(&conn->args, "-6");
- if (tortoiseplink)
+ if (needs_batch)
argv_array_push(&conn->args, "-batch");
if (port) {
- /* P is for PuTTY, p is for OpenSSH */
- argv_array_push(&conn->args, putty ? "-P" : "-p");
+ argv_array_pushf(&conn->args, "-%c", port_option);
argv_array_push(&conn->args, port);
}
argv_array_push(&conn->args, ssh_host);
--
2.11.0-699-ga1f1475476
^ permalink raw reply related
* Re: SubmittingPatches: drop temporal reference for PGP signing
From: Stefan Beller @ 2017-01-25 22:38 UTC (permalink / raw)
To: Philip Oakley
Cc: Cornelius Weig, Johannes Sixt, bitte.keine.werbung.einwerfen,
git@vger.kernel.org, Junio C Hamano, thomas.braun, John Keeping
In-Reply-To: <33E354BCDB9A4192B69B9B399381659E@PhilipOakley>
On Tue, Jan 24, 2017 at 10:54 PM, Philip Oakley <philipoakley@iee.org> wrote:
>> -Do not PGP sign your patch, at least for now. Most likely, your
>> -maintainer or other people on the list would not have your PGP
>> -key and would not bother obtaining it anyway. Your patch is not
>> -judged by who you are; a good patch from an unknown origin has a
>> -far better chance of being accepted than a patch from a known,
>> -respected origin that is done poorly or does incorrect things.
>> +Do not PGP sign your patch. Most likely, your maintainer or other
>> +people on the list would not have your PGP key and would not bother
>> +obtaining it anyway. Your patch is not judged by who you are; a good
>> +patch from an unknown origin has a far better chance of being accepted
>> +than a patch from a known, respected origin that is done poorly or
>> +does incorrect things.
>
>
> Wouldn't this also benefit from a forward reference to the section 5 on the
> DOC signining? This would avoid Cornelius's case where he felt that section
> 5 no longer applied.
Yeah I agree. My patch was not the best shot by far.
^ permalink raw reply
* Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND
From: Jeff King @ 2017-01-25 22:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git, Segev Finer
In-Reply-To: <xmqqinp2939j.fsf@gitster.mtv.corp.google.com>
On Wed, Jan 25, 2017 at 02:35:36PM -0800, Junio C Hamano wrote:
> -- >8 --
> Subject: [PATCH] connect: core.sshvariant to correct misidentification
I have been watching this discussion from the sidelines, and I agree
that this direction is a big improvement.
> +static void override_ssh_variant(int *port_option, int *needs_batch)
> +{
> + const char *variant;
> +
> + if (git_config_get_string_const("core.sshvariant", &variant))
> + return;
> + if (!strcmp(variant, "tortoiseplink")) {
> + *port_option = 'P';
> + *needs_batch = 1;
> + } else if (!strcmp(variant, "putty")) {
> + *port_option = 'P';
> + *needs_batch = 0;
> + } else {
> + /* default */
> + if (strcmp(variant, "ssh")) {
> + warning(_("core.sshvariant: unknown value '%s'"), variant);
> + warning(_("using OpenSSH compatible behaviour"));
> + }
> + *port_option = 'p';
> + *needs_batch = 0;
> + }
> +}
IIRC, the "const" in git_config_get_string_const is only about avoiding
an annoying cast. The result is still allocated and needs freed. Since
you are not keeping the value after the function returns, I think you
could just use git_config_get_value().
(Grepping around, I see a few other places that seem to make the same
mistake. I think this is a confusing interface that should probably be
fixed).
-Peff
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2017, #04; Mon, 23)
From: Junio C Hamano @ 2017-01-25 22:51 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, Lars Schneider, git
In-Reply-To: <20170125183924.6yclcjl4ggcu42yp@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I guess the way to dig would be to add a test that looks at the output
> of "type mv" or something, push it to a Travis-hooked branch, and then
> wait for the output
Sounds tempting ;-)
^ permalink raw reply
* Re: Force Confirmation for Dropping Changed Lines
From: Hilco Wijbenga @ 2017-01-25 22:51 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git Users
In-Reply-To: <CA+P7+xrupLuYAj7tn_1EaUiN6eaCmtgX-_d4mnByDq95cuqiWQ@mail.gmail.com>
On 25 January 2017 at 14:24, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Wed, Jan 25, 2017 at 2:16 PM, Hilco Wijbenga
> <hilco.wijbenga@gmail.com> wrote:
>> How can I force Git to not assume my change to the first line is "redundant"?
>>
>
> My guess is that you probably want a custom merge driver for your file
> types. That's where I would look initially.
Mmm, that sounds complex. The "my-code.x" is made up so I could keep
my example as simple as possible. In reality, it's Maven's POM files
(pom.xml).
So there is no setting for any of this? There is no way to switch off
auto merging for certain files?
^ permalink raw reply
* Re: Force Confirmation for Dropping Changed Lines
From: Jacob Keller @ 2017-01-25 22:54 UTC (permalink / raw)
To: Hilco Wijbenga; +Cc: Git Users
In-Reply-To: <CAE1pOi3eh7ao3NocV=PRFDby8y5ttjFR=-_VB0FoJv4MpjExaA@mail.gmail.com>
On Wed, Jan 25, 2017 at 2:51 PM, Hilco Wijbenga
<hilco.wijbenga@gmail.com> wrote:
> On 25 January 2017 at 14:24, Jacob Keller <jacob.keller@gmail.com> wrote:
>> On Wed, Jan 25, 2017 at 2:16 PM, Hilco Wijbenga
>> <hilco.wijbenga@gmail.com> wrote:
>>> How can I force Git to not assume my change to the first line is "redundant"?
>>>
>>
>> My guess is that you probably want a custom merge driver for your file
>> types. That's where I would look initially.
>
> Mmm, that sounds complex. The "my-code.x" is made up so I could keep
> my example as simple as possible. In reality, it's Maven's POM files
> (pom.xml).
>
> So there is no setting for any of this? There is no way to switch off
> auto merging for certain files?
Not really sure, but a quick google search revealed
https://github.com/ralfth/pom-merge-driver
Maybe that will help you?
Thanks,
Jake
^ permalink raw reply
* Re: [PATCHv2 3/3] submodule absorbing: fix worktree/gitdir pointers recursively for non-moves
From: Junio C Hamano @ 2017-01-25 22:54 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill, peff
In-Reply-To: <20170124235651.18749-4-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> Consider having a submodule 'sub' and a nested submodule at 'sub/nested'.
> When nested is already absorbed into sub, but sub is not absorbed into
> its superproject, then we need to fixup the gitfile and core.worktree
> setting for 'nested' when absorbing 'sub', but we do not need to move
> its git dir around.
>
> Previously 'nested's gitfile contained "gitdir: ../.git/modules/nested";
> it has to be corrected to "gitdir: ../../.git/modules/sub1/modules/nested".
That sounds like a sensible way to make things consistent.
^ permalink raw reply
* Re: [PATCH] tag: add tag.createReflog option
From: Junio C Hamano @ 2017-01-25 22:56 UTC (permalink / raw)
To: Jeff King; +Cc: Cornelius Weig, git, novalis, pclouds
In-Reply-To: <xmqqpoja95o5.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Jeff King <peff@peff.net> writes:
>
>> +enum log_refs_config {
>> + LOG_REFS_UNSET = -1,
>> + LOG_REFS_NONE = 0,
>> + LOG_REFS_NORMAL, /* see should_create_reflog for rules */
>> + LOG_REFS_ALWAYS
>> +};
>> +extern enum log_refs_config log_all_ref_updates;
>> +...
>> +int should_create_reflog(const char *refname)
>> +{
>> + switch (log_all_ref_updates) {
>> + case LOG_REFS_ALWAYS:
>> + return 1;
>> + case LOG_REFS_NORMAL:
>> + return !prefixcmp(refname, "refs/heads/") ||
>> + !prefixcmp(refname, "refs/remotes/") ||
>> + !prefixcmp(refname, "refs/notes/") ||
>> + !strcmp(refname, "HEAD");
>> + default:
>> + return 0;
>> + }
>> +}
>
> Yup, this is how I expected for the feature to be done.
>
> Just a hint for Cornelius; prefixcmp() is an old name for what is
> called starts_with() these days.
It may have been obvious, but to be explicit for somebody new,
!prefixcmp() corresponds to starts_with(). IOW, we changed the
meaning of the return value when moving from cmp-lookalike (where 0
means "equal") to "does it start with this string?" bool (where 1
means "yes").
^ permalink raw reply
* Re: [PATCHv2 3/3] submodule absorbing: fix worktree/gitdir pointers recursively for non-moves
From: Stefan Beller @ 2017-01-25 23:04 UTC (permalink / raw)
To: Brandon Williams; +Cc: Junio C Hamano, git@vger.kernel.org, Jeff King
In-Reply-To: <20170125004610.GC58021@google.com>
On Tue, Jan 24, 2017 at 4:46 PM, Brandon Williams <bmwill@google.com> wrote:
> On 01/24, Stefan Beller wrote:
>> + connect_work_tree_and_git_dir(path, real_new_git_dir);
>
> Memory leak with 'real_new_git_dir'
yup. :(
> The scope of 'real_sub_git_dir' and 'real_common_git_dir' variable can
> be narrowed. Move their declaration here and free it prior to exiting
> the else block.
ok.
> 'v' isn't ever used, just use starts_with() and get rid of 'v'
makes sense.
>
>> + relocate_single_git_dir_into_superproject(prefix, path);
>> + }
>>
>
> There's a label 'out' at the end of the function which can be removed as
> there is no 'goto' statement using it.
We do use it in
if (err_code == READ_GITFILE_ERR_STAT_FAILED)
goto out; /* unpopulated as expected */
I took your proposed SQUASH and removed the goto, see the followup email.
^ permalink raw reply
* Re: [PATCH] gpg-interface: Add some output from gpg when it errors out.
From: Junio C Hamano @ 2017-01-25 23:04 UTC (permalink / raw)
To: Mike Hommey; +Cc: git
In-Reply-To: <20170125030434.26448-1-mh@glandium.org>
Mike Hommey <mh@glandium.org> writes:
> For instance, after changing my laptop for a new one, I copied my
> configs, but had some environment differences that broke gpg.
> With this change applied, the output becomes, on this new machine:
> gpg: keyblock resource '/usr/share/keyrings/debian-keyring.gpg': No
> such file or directory
> error: gpg failed to sign the data
> error: unable to sign the tag
>
> which makes it clearer what's wrong.
Overall I think this is a good thing to do. Instead of eating the
status output, showing what we got, especially when we know the
command failed, would make the bug-hunting of user's environment
easier, like you showed above.
The only thing in the design that makes me wonder is the filtering
out based on "[GNUPG:]" prefix. Why do we need to filter them out?
Implementation-wise, I'd be happier if we do not add any new
callsites of strbuf_split(), which is a horrible interface. But
that is a minor detail.
Thanks.
^ permalink raw reply
* [PATCHv3 3/3] submodule absorbing: fix worktree/gitdir pointers recursively for non-moves
From: Stefan Beller @ 2017-01-25 23:04 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, peff, Stefan Beller
In-Reply-To: <xmqq37g692da.fsf@gitster.mtv.corp.google.com>
Consider having a submodule 'sub' and a nested submodule at 'sub/nested'.
When nested is already absorbed into sub, but sub is not absorbed into
its superproject, then we need to fixup the gitfile and core.worktree
setting for 'nested' when absorbing 'sub', but we do not need to move
its git dir around.
Previously 'nested's gitfile contained "gitdir: ../.git/modules/nested";
it has to be corrected to "gitdir: ../../.git/modules/sub1/modules/nested".
An alternative I considered to do this work lazily, i.e. when resolving
"../.git/modules/nested", we would notice the ".git" being a gitfile
linking to another path. That seemed to be robuster by design, but harder
to get the implementation right. Maybe we have to do that anyway once we
try to have submodules and worktrees working nicely together, but for now
just produce 'correct' (i.e. direct) pointers.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
This replaces the last patch of the series, containing Brandons SQUASH proposal
as well as the removal of the goto.
Thanks,
Stefan
submodule.c | 62 ++++++++++++++++++++++++++++----------
t/t7412-submodule-absorbgitdirs.sh | 27 +++++++++++++++++
2 files changed, 73 insertions(+), 16 deletions(-)
diff --git a/submodule.c b/submodule.c
index 4c4f033e8a..3b98766a6b 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1437,22 +1437,57 @@ void absorb_git_dir_into_superproject(const char *prefix,
const char *path,
unsigned flags)
{
- const char *sub_git_dir, *v;
- char *real_sub_git_dir = NULL, *real_common_git_dir = NULL;
+ int err_code;
+ const char *sub_git_dir;
struct strbuf gitdir = STRBUF_INIT;
-
strbuf_addf(&gitdir, "%s/.git", path);
- sub_git_dir = resolve_gitdir(gitdir.buf);
+ sub_git_dir = resolve_gitdir_gently(gitdir.buf, &err_code);
/* Not populated? */
- if (!sub_git_dir)
- goto out;
+ if (!sub_git_dir) {
+ char *real_new_git_dir;
+ const char *new_git_dir;
+ const struct submodule *sub;
+
+ if (err_code == READ_GITFILE_ERR_STAT_FAILED) {
+ /* unpopulated as expected */
+ strbuf_release(&gitdir);
+ return;
+ }
+
+ if (err_code != READ_GITFILE_ERR_NOT_A_REPO)
+ /* We don't know what broke here. */
+ read_gitfile_error_die(err_code, path, NULL);
+
+ /*
+ * Maybe populated, but no git directory was found?
+ * This can happen if the superproject is a submodule
+ * itself and was just absorbed. The absorption of the
+ * superproject did not rewrite the git file links yet,
+ * fix it now.
+ */
+ sub = submodule_from_path(null_sha1, path);
+ if (!sub)
+ die(_("could not lookup name for submodule '%s'"), path);
+ new_git_dir = git_path("modules/%s", sub->name);
+ if (safe_create_leading_directories_const(new_git_dir) < 0)
+ die(_("could not create directory '%s'"), new_git_dir);
+ real_new_git_dir = real_pathdup(new_git_dir);
+ connect_work_tree_and_git_dir(path, real_new_git_dir);
+
+ free(real_new_git_dir);
+ } else {
+ /* Is it already absorbed into the superprojects git dir? */
+ char *real_sub_git_dir = real_pathdup(sub_git_dir);
+ char *real_common_git_dir = real_pathdup(get_git_common_dir());
- /* Is it already absorbed into the superprojects git dir? */
- real_sub_git_dir = real_pathdup(sub_git_dir);
- real_common_git_dir = real_pathdup(get_git_common_dir());
- if (!skip_prefix(real_sub_git_dir, real_common_git_dir, &v))
- relocate_single_git_dir_into_superproject(prefix, path);
+ if (!starts_with(real_sub_git_dir, real_common_git_dir))
+ relocate_single_git_dir_into_superproject(prefix, path);
+
+ free(real_sub_git_dir);
+ free(real_common_git_dir);
+ }
+ strbuf_release(&gitdir);
if (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {
struct child_process cp = CHILD_PROCESS_INIT;
@@ -1478,9 +1513,4 @@ void absorb_git_dir_into_superproject(const char *prefix,
strbuf_release(&sb);
}
-
-out:
- strbuf_release(&gitdir);
- free(real_sub_git_dir);
- free(real_common_git_dir);
}
diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh
index 1c47780e2b..e2bbb449b6 100755
--- a/t/t7412-submodule-absorbgitdirs.sh
+++ b/t/t7412-submodule-absorbgitdirs.sh
@@ -64,6 +64,33 @@ test_expect_success 'absorb the git dir in a nested submodule' '
test_cmp expect.2 actual.2
'
+test_expect_success 're-setup nested submodule' '
+ # un-absorb the direct submodule, to test if the nested submodule
+ # is still correct (needs a rewrite of the gitfile only)
+ rm -rf sub1/.git &&
+ mv .git/modules/sub1 sub1/.git &&
+ GIT_WORK_TREE=. git -C sub1 config --unset core.worktree &&
+ # fixup the nested submodule
+ echo "gitdir: ../.git/modules/nested" >sub1/nested/.git &&
+ GIT_WORK_TREE=../../../nested git -C sub1/.git/modules/nested config \
+ core.worktree "../../../nested" &&
+ # make sure this re-setup is correct
+ git status --ignore-submodules=none
+'
+
+test_expect_success 'absorb the git dir in a nested submodule' '
+ git status >expect.1 &&
+ git -C sub1/nested rev-parse HEAD >expect.2 &&
+ git submodule absorbgitdirs &&
+ test -f sub1/.git &&
+ test -f sub1/nested/.git &&
+ test -d .git/modules/sub1/modules/nested &&
+ git status >actual.1 &&
+ git -C sub1/nested rev-parse HEAD >actual.2 &&
+ test_cmp expect.1 actual.1 &&
+ test_cmp expect.2 actual.2
+'
+
test_expect_success 'setup a gitlink with missing .gitmodules entry' '
git init sub2 &&
test_commit -C sub2 first &&
--
2.11.0.495.g04f60290a0.dirty
^ permalink raw reply related
* Re: [PATCH v2 0/7] Macros for Asciidoctor support
From: brian m. carlson @ 2017-01-25 23:19 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, git
In-Reply-To: <20170125213544.eelk4pjhrhshi6zh@sigill.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 938 bytes --]
On Wed, Jan 25, 2017 at 04:35:44PM -0500, Jeff King wrote:
> On Wed, Jan 25, 2017 at 02:28:55PM +0100, Johannes Schindelin wrote:
>
> > > The need for the extensions could be replaced with a small amount of
> > > Ruby code, if that's considered desirable. Previous opinions on doing
> > > so were negative, however.
> >
> > Quite frankly, it is annoying to be forced to install the extensions. I
> > would much rather have the small amount of Ruby code in Git's repository.
>
> Me too. Dependencies can be a big annoyance. I'd reserve judgement until
> I saw the actual Ruby code, though. :)
I've sent the patch before, but I can send it again. It's relatively
small and self-contained. I'm also happy to be responsible for
maintaining it.
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* Re: [PATCH v2 25/27] attr: store attribute stack in attr_check structure
From: Brandon Williams @ 2017-01-25 23:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, sbeller, pclouds
In-Reply-To: <20170125215426.GB83343@google.com>
On 01/25, Brandon Williams wrote:
> On 01/25, Junio C Hamano wrote:
> > Brandon Williams <bmwill@google.com> writes:
> >
> > >> In my mind, the value of having a constant check_attr is primarily
> > >> that it gives us a stable pointer to serve as a hashmap key,
> > >> i.e. the identifier for each call site, in a later iteration.
> > >
> > > We didn't really discuss this notion of having the pointer be a key into
> > > a hashmap, what sort of information are you envisioning being stored in
> > > this sort of hashmap?
> >
> > The "entries relevant to this attr_check() call, that is specific to
> > the <check_attr instance, the thread> tuple" (aka "what used to be
> > called the global attr_stack") we discussed would be the primary
> > example. A thread is likely be looping in a caller that has many
> > paths inside a directory, calling a function that has a call to
> > attr_check() for each path. Having something that can use to
> > identify the check_attr instance in a stable way, even when the
> > inner function is called and returns many times, would allow us to
> > populate the "attr stack" just once for the thread when it enters a
> > directory for the first time (remember, another thread may be
> > executing the same codepath, checking for paths in a different
> > directory) and keep using it. There may be other mechanisms you can
> > come up with, so I wouldn't say it is the only valid way, but it is
> > a way. That is why I said:
> >
> > >> But all of the above comes from my intuition, and I'll very much
> > >> welcome to be proven wrong with an alternative design, or better
> > >> yet, a working code based on an alternative design ;-).
> >
> > near the end of my message.
> >
> > > One issue I can see with this is that the
> > > functions which have a static attr_check struct would still not be thread
> > > safe if the initialization of the structure isn't surrounded by a mutex
> > > itself. ie
> >
> > Yes, that goes without saying. That is why I suggested Stefan to do
> > not this:
> >
> > > static struct attr_check *check;
> > >
> > > if (!check)
> > > init(check);
> > >
> > > would need to be:
> > >
> > > lock()
> > > if (!check)
> > > init(check);
> > > unlock();
> >
> > but this:
> >
> > static struct attr_check *check;
> > init(&check);
> >
> > and hide the lock/unlock gymnastics inside the API. I thought that
> > already was in what you inherited from him and started your work
> > on top of?
>
> I essentially built off of the series you had while using Stefan's
> patches as inspiration, but I don't believe the kind of mechanism you
> are describing existed in Stefan's series. His series had a single lock
> for the entire system, only allowing a single caller to be in it at any
> given time. This definitely isn't ideal, hence why I picked it up.
>
> Implementation aside I want to try and nail down what the purpose of
> this refactor is. There are roughly two notions of being "thread-safe".
>
> 1. The first is that the subsystem itself is thread safe, that is
> multiple threads can be executing inside the subsystem without stepping
> on each others work.
>
> 2. The second is that the object itself is thread safe or that multiple
> threads can use the same object.
>
> I thought that the main purpose of this was to achieve (1) since
> currently that is not the case.
Ok, so I discovered a very good reason why we should do as Stefan
originally did and split the question and answer (beyond the reasoning
for using the reference as a hashkey).
One motivation behind making this API thread-safe is that we can use it
in pathspec code to match against attributes. This means that a
pathspec structure will contain an attr_check member describing the
attributes that a pathspec item is interested in. Then the pathspec
structure is passed to match_pathspec() as a const pointer. To me, when
passing something as 'const' I expect none of the members should change
at all. The struct should remain exactly in the same form as before I
invoked the function.
Requiring the attr_check structure to be modified in the process of a
check_attrs() call would violate this "contract" when calling
match_pathspec() as the attr_check structure would have modified state.
The compiler wouldn't catch this as the "const" modifier isn't passed on
to struct members.
--
Brandon Williams
^ permalink raw reply
* Re: diff --color-words breaks spacing
From: Jeff King @ 2017-01-25 23:20 UTC (permalink / raw)
To: Phil Hord; +Cc: Git
In-Reply-To: <CABURp0ryBtLALEnzRdoeYeUUdrzBxiT3+yTvW6B=vpqMG3p1Rw@mail.gmail.com>
On Tue, Jan 24, 2017 at 09:39:31AM -0800, Phil Hord wrote:
> I noticed some weird spacing when comparing files with git diff
> --color-words. The space before a colored word disappears sometimes.
I _think_ this is working as designed, though it is a bit tricky (and it
may be possible to make it better).
> echo "FOO foo; foo = bar" > a
> echo "FOO foo = baz" > b
> git diff --color-words --no-index a b
> FOOfoo; foo = barbaz
It might be easier to see with --word-diff, which uses the same code but
marks it with punctuation:
$ git diff --word-diff
FOO[-foo;-] foo = [-bar-]{+baz+}
The key thing is that what you are seeing is the post-image, plus
word-based annotations. And the post-image does not have that space in
its context, so we would not expect to see:
FOO [-foo;-] foo = [-bar-]{+baz+}
(you would if we showed the pre-image plus annotations; but then I think
you'd probably end up with the opposite problem when text was added).
However, we also don't see the space in the removed part. I.e., I think:
FOO[- foo;-] foo = [-bar-]{+baz+}
would be correct. But I think because of the way word-diff is
implemented, a non-word character will never be part of the annotation.
The way it works is basically to break the string down on word
boundaries, so we have:
pre: FOO, foo;, foo, =, bar
post: FOO, foo, =, baz
as a set of tokens. We stick each of those on their own line and feed
them back to xdiff, so we get a diff like:
@@ -1,5 +1,4 @@
FOO
-foo;
foo
=
-bar
+baz
and then walk through the post-image buffer, either inserting chunks of
context from the original buffer, or the changed lines. But the changed
lines themselves do not include the non-word characters, so we have no
idea that a space went away.
-Peff
^ permalink raw reply
* Re: [PATCH] Retire the `relink` command
From: Eric Wong @ 2017-01-25 23:20 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <10319c47ff3f7222c3a601827ebd9398861d509d.1485363528.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
> Back in the olden days, when all objects were loose and rubber boots were
> made out of wood, it made sense to try to share (immutable) objects
> between repositories.
>
> Ever since the arrival of pack files, it is but an anachronism.
>
> Let's move the script to the contrib/examples/ directory and no longer
> offer it.
On the other hand, we have no idea if there are still people
using it for whatever reason...
I suggest we have a deprecation period where:
1) there is warning message when it's run
2) a note in the manpage indicating it's to-be-removed
3) ...then wait a few distro LTS cycles to remove it entirely
^ permalink raw reply
* Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND
From: Junio C Hamano @ 2017-01-25 23:25 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, git, Segev Finer
In-Reply-To: <20170125224043.jxjc4avuy4ztnkm4@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Jan 25, 2017 at 02:35:36PM -0800, Junio C Hamano wrote:
>
>> -- >8 --
>> Subject: [PATCH] connect: core.sshvariant to correct misidentification
>
> I have been watching this discussion from the sidelines, and I agree
> that this direction is a big improvement.
> ...
> IIRC, the "const" in git_config_get_string_const is only about avoiding
> an annoying cast. The result is still allocated and needs freed. Since
> you are not keeping the value after the function returns, I think you
> could just use git_config_get_value().
It is too late for today's pushout (I have this topic near the tip
of 'pu', so it is possible to tweak and redo only 'pu' branch, but I
generally hate tweaking things after 15:00 my time), but I'll fix
that before the topic hits 'jch' (which is a bit more than 'next'
and that is what I use for everyday work).
Thanks.
^ permalink raw reply
* Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude
From: Junio C Hamano @ 2017-01-25 23:25 UTC (permalink / raw)
To: Jacob Keller
Cc: Jeff King, Nguyễn Thái Ngọc Duy,
Git mailing list
In-Reply-To: <CA+P7+xovOx9ebo6MU0e4v==+76jtoMXz45+LnBPFifHbjqFU4w@mail.gmail.com>
Jacob Keller <jacob.keller@gmail.com> writes:
> On Wed, Jan 25, 2017 at 1:27 PM, Jeff King <peff@peff.net> wrote:
>> On Wed, Jan 25, 2017 at 03:57:18PM -0500, Jeff King wrote:
>>
>>> IOW, the ref-selector options build up until a group option is given,
>>> which acts on the built-up options (over that group) and then resets the
>>> built-up options. Doing "--unrelated" as above is orthogonal (though I
>>> think in practice nobody would do that, because it's hard to read).
>>
>> So here's what I would have expected your series to look more like (with
>> probably one patch adding clear_ref_selection_options, and the other
>> adding the decorate stuff):
>>
>
> I agree that this is how I would have expected it to work as well.
That makes three of us ;-)
^ 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