* Re: [PATCHv2] attr: convert to new threadsafe API
From: Junio C Hamano @ 2016-10-12 16:20 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Brandon Williams
In-Reply-To: <CAGZ79kZrNSmPAQ6SmBzFDJtSmdCbqKcgQu4KDLfoYVkSXvo-og@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
>> I am not sure if the updates to the callers fulfill that purpose.
>> For example, look at this hunk.
>>
>>> @@ -111,6 +111,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
>>> struct archiver_args *args = c->args;
>>> write_archive_entry_fn_t write_entry = c->write_entry;
>>> static struct git_attr_check *check;
>>> + static struct git_attr_result result;
>>
>> As we discussed, this caller, even when threaded, will always want
>> to ask for a fixed two attributes, so "check" being static and
>> shared across threads is perfectly fine. But we do not want to see
>> "result" shared, do we?
>
> Well all of the hunks in the patch are not threaded, so they
> don't follow a threading pattern, but the static pattern to not be
> more expensive than needed.
Is it too invasive a change to make them as if they are thread-ready
users of API that happen to know their callers are not threading?
It would be ideal if we can prepare them so that the way they
interact with the attr subsystem will not have to change after this
step.
>> In other words, ideally, I think this part of the patch should
>> rather read like this:
>>
>> static struct git_attr_check *check;
>> struct git_attr_result result[2];
>>
>> ...
>> git_attr_check_initl(&check, "export-ignore", "export-subst", NULL);
>> if (!git_check_attr(path_without_prefix, check, result)) {
>> ... use result[0] and result[1] ...
>>
>> For sanity checking, it is OK to add ARRAY_SIZE(result) as the final
>> and extra parameter to git_check_attr() so that the function can
>> make sure it matches (or exceeds) check->nr.
>
> That seems tempting from a callers perspective; I'll look into that.
For callers that prepare "check" and "result" before asking
check-attr about the attributes in "check" for many paths, it is OK
to use your "allocate with attr_result_init()" pattern. The "result"
still needs to be made non-static, though.
But many callers do not follow that; rather they do
loop to iterate over paths {
call a helper func to learn attr X for path
use the value of attr X
}
using a callchain that embeds a helper function deep inside, and
"check" is kept in the helper, check-attr function is called from
there, and "result" is not passed from the caller to the helper
(obviously, because it does not exist in the current API). See the
callchain that leads down to convert.c::convert_attrs() for a
typical example. When converted to the new API, it needs to have a
new "result" structure every time it is called, and cannot reuse the
one that was used in its previous call.
^ permalink raw reply
* 2 directories same spelling one directory is camel cased
From: David Brown @ 2016-10-12 16:05 UTC (permalink / raw)
To: git
Howdy git gurus,
I have the dubious distinction of working with a remote repo (master)
that has a class loader run-time error when cloned, built and executed.
The reason for the runtime issue is a directory hierarchical path has to
directories (folders) with the same name spelling but one of the
directories is camel-cased. The package names are the same.
The compiler doesn't care but the run-time class loader has an issue
with the 2 'same like named' classes.
How to remove the offending directories and files at the locally cloned
repo but not push 'deleted' directories and files back to origin/master
the remote repo?
Please advise.
Regards.
^ permalink raw reply
* Re: interactive rebase should better highlight the not-applying commit
From: Johannes Schindelin @ 2016-10-12 16:14 UTC (permalink / raw)
To: Stefan Beller; +Cc: Joshua N Pritikin, git@vger.kernel.org
In-Reply-To: <CAGZ79kYg3sZ42W-PEE7MgCDvt_h7hEQ7KWZsVKMb3DY=x5VK+w@mail.gmail.com>
Hi Stefan,
On Tue, 11 Oct 2016, Stefan Beller wrote:
> On Tue, Oct 11, 2016 at 12:07 PM, Joshua N Pritikin <jpritikin@pobox.com> wrote:
> > I assume somebody familiar with GIT's code base could make this change
> > in about 10 minutes.
>
> Can you elaborate how you come to that estimate?
Why do you ask? He obviously has "a very good brain" ;-)
Seriously again, Git's source code is not that hard to read, and the Git
developer community is pretty helpful when anybody asks for pointers what
code to change.
Having said that, I did reimplement some parts of the shell script that is
git-rebase--interactive.sh [*1*] in C and am in the process of getting
those integrated into the next (or hopefully not a *much* later) version
of Git.
So what I'd like to see is an *exact* copy-paste of a message in question,
and a *concrete* proposal how it should look like instead.
Ciao,
Johannes
Footnote *1*:
https://github.com/git/git/blob/master/git-rebase--interactive.sh
^ permalink raw reply
* Re: Formatting problem send_mail in version 2.10.0
From: Larry Finger @ 2016-10-12 15:59 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Jeff King, Mathieu Lienard--Mayor, Remi Lespinet, git
In-Reply-To: <vpq8tttr2ps.fsf@anie.imag.fr>
On 10/12/2016 10:45 AM, Matthieu Moy wrote:
> Larry Finger <Larry.Finger@lwfinger.net> writes:
>
>> On 10/12/2016 02:36 AM, Matthieu Moy wrote:
>>> Larry Finger <Larry.Finger@lwfinger.net> writes:
>>>
>>>> On 10/11/2016 11:18 AM, Matthieu Moy wrote:
>>>>> Larry Finger <Larry.Finger@lwfinger.net> writes:
>>>>>
>>>>>> That added information at the end is intended to be passed on to the
>>>>>> stable group. In this case, the patch needs to be applied to kernel
>>>>>> versions 4.8 and later.
>>>>>
>>>>> OK, but where do people fetch this information from?
>>>>
>>>> This format is used in a patch for the kernel. When the patch is
>>>> merged into mainline, stable@vger.kernel.org gets sent an E-mail with
>>>> a copy of the original patch. Maintainers of the indicated systems
>>>> then merge the patch with their stable version.
>>>
>>> Sorry, but this does not answer my question. I'll rephrase: when
>>> people behind stable@vger.kernel.org get the message, how do they know
>>> which version of the kernel they should apply it to?
>>>
>>>> I do not want it in the body of the message. I just want to pass a
>>>> hint to the stable maintainer(s).
>>>
>>> If it's not in the body of the message, then where is it?
>>
>> This point is clarified in the thread
>> http://marc.info/?l=linux-wireless&m=147625930203434&w=2, which is
>> with my upstream maintainer.
>
> Which explicitly states that the syntax is not [$number], but # $number,
> right?
Yes it does; however v2.10.0 mangles this into
Stable <stable@vger.kernel.org#4.8+>
and my mailer refuses to send that.
Larry
^ permalink raw reply
* Re: Formatting problem send_mail in version 2.10.0
From: Matthieu Moy @ 2016-10-12 15:45 UTC (permalink / raw)
To: Larry Finger; +Cc: Jeff King, Mathieu Lienard--Mayor, Remi Lespinet, git
In-Reply-To: <5978b674-02eb-59ad-9468-93203533575f@lwfinger.net>
Larry Finger <Larry.Finger@lwfinger.net> writes:
> On 10/12/2016 02:36 AM, Matthieu Moy wrote:
>> Larry Finger <Larry.Finger@lwfinger.net> writes:
>>
>>> On 10/11/2016 11:18 AM, Matthieu Moy wrote:
>>>> Larry Finger <Larry.Finger@lwfinger.net> writes:
>>>>
>>>>> That added information at the end is intended to be passed on to the
>>>>> stable group. In this case, the patch needs to be applied to kernel
>>>>> versions 4.8 and later.
>>>>
>>>> OK, but where do people fetch this information from?
>>>
>>> This format is used in a patch for the kernel. When the patch is
>>> merged into mainline, stable@vger.kernel.org gets sent an E-mail with
>>> a copy of the original patch. Maintainers of the indicated systems
>>> then merge the patch with their stable version.
>>
>> Sorry, but this does not answer my question. I'll rephrase: when
>> people behind stable@vger.kernel.org get the message, how do they know
>> which version of the kernel they should apply it to?
>>
>>> I do not want it in the body of the message. I just want to pass a
>>> hint to the stable maintainer(s).
>>
>> If it's not in the body of the message, then where is it?
>
> This point is clarified in the thread
> http://marc.info/?l=linux-wireless&m=147625930203434&w=2, which is
> with my upstream maintainer.
Which explicitly states that the syntax is not [$number], but # $number,
right?
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: Formatting problem send_mail in version 2.10.0
From: Matthieu Moy @ 2016-10-12 15:40 UTC (permalink / raw)
To: Larry Finger; +Cc: Jeff King, Mathieu Lienard--Mayor, Remi Lespinet, git
In-Reply-To: <02cfe81b-9f6a-ddbd-3411-2839e8a58b1e@lwfinger.net>
Larry Finger <Larry.Finger@lwfinger.net> writes:
> On 10/12/2016 02:36 AM, Matthieu Moy wrote:
>> Larry Finger <Larry.Finger@lwfinger.net> writes:
>>
>>> On 10/11/2016 11:18 AM, Matthieu Moy wrote:
>>>> Larry Finger <Larry.Finger@lwfinger.net> writes:
>>>>
>>>>> That added information at the end is intended to be passed on to the
>>>>> stable group. In this case, the patch needs to be applied to kernel
>>>>> versions 4.8 and later.
>>>>
>>>> OK, but where do people fetch this information from?
>>>
>>> This format is used in a patch for the kernel. When the patch is
>>> merged into mainline, stable@vger.kernel.org gets sent an E-mail with
>>> a copy of the original patch. Maintainers of the indicated systems
>>> then merge the patch with their stable version.
>>
>> Sorry, but this does not answer my question. I'll rephrase: when
>> people behind stable@vger.kernel.org get the message, how do they know
>> which version of the kernel they should apply it to?
>>
>>> I do not want it in the body of the message. I just want to pass a
>>> hint to the stable maintainer(s).
>>
>> If it's not in the body of the message, then where is it?
>
> That information is supplied in the "Cc: Stable" line at the end.
OK, so it *is* in the body of the email message.
If I understand correctly, the important is that:
* "git send-email" should accept sending a message for a commit
containing a Cc: <address@host> [number] line.
* The message should be sent to <address@host>
But I still don't get why using the (4.8+) notation wouldn't work. It
would still end up in the Cc: line in the body of the message, and the
message would still be sent to the same address. It is *not* dropped
from the message, just from the email headers of the message being sent.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: Formatting problem send_mail in version 2.10.0
From: Larry Finger @ 2016-10-12 15:40 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Jeff King, Mathieu Lienard--Mayor, Remi Lespinet, git
In-Reply-To: <vpq60oy589b.fsf@anie.imag.fr>
On 10/12/2016 02:36 AM, Matthieu Moy wrote:
> Larry Finger <Larry.Finger@lwfinger.net> writes:
>
>> On 10/11/2016 11:18 AM, Matthieu Moy wrote:
>>> Larry Finger <Larry.Finger@lwfinger.net> writes:
>>>
>>>> That added information at the end is intended to be passed on to the
>>>> stable group. In this case, the patch needs to be applied to kernel
>>>> versions 4.8 and later.
>>>
>>> OK, but where do people fetch this information from?
>>
>> This format is used in a patch for the kernel. When the patch is
>> merged into mainline, stable@vger.kernel.org gets sent an E-mail with
>> a copy of the original patch. Maintainers of the indicated systems
>> then merge the patch with their stable version.
>
> Sorry, but this does not answer my question. I'll rephrase: when
> people behind stable@vger.kernel.org get the message, how do they know
> which version of the kernel they should apply it to?
>
>> I do not want it in the body of the message. I just want to pass a
>> hint to the stable maintainer(s).
>
> If it's not in the body of the message, then where is it?
This point is clarified in the thread
http://marc.info/?l=linux-wireless&m=147625930203434&w=2, which is with my
upstream maintainer.
Larry
^ permalink raw reply
* Re: [PATCH 1/5] trailer: use singly-linked list, not doubly
From: Christian Couder @ 2016-10-12 15:38 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, Junio C Hamano
In-Reply-To: <8e12e0954f0a23d7c7905c58a3f7d8084d9338be.1476232683.git.jonathantanmy@google.com>
On Wed, Oct 12, 2016 at 3:23 AM, Jonathan Tan <jonathantanmy@google.com> wrote:
> Use singly-linked lists (instead of doubly-linked lists) in trailer to
> keep track of arguments (whether implicit from configuration or explicit
> from the command line) and trailer items.
>
> This change significantly reduces the code length and simplifies the code.
It's true that the code can be simplified a lot by using a
singly-linked list, but if we already had and used some generic
functions or macros to handle doubly-linked list, I doubt there would
be a significant simplification (as the generic code could not be
deleted in this case).
> There are now fewer pointers to be manipulated, but most trailer
> manipulations now require seeking from beginning to end, so there might
> be a slight net decrease in performance; however the number of trailers
> is usually small (10 to 15 at the most) so this should not cause a big
> impact.
By default we append new trailers at the end of the trailer list, so a
singly-linked list is theoretically not well suited at all for
handling trailer lists.
You say that at most there is a small number of trailers, and that may
be true indeed for the Linux kernel and most projects these days, but
I am not sure it is a good assumption to make in general.
For example if some projects use or start using "CC: *" trailers and
tools to automatically append such trailers (perhaps using 'git
interpret-trailers' for that purpose by the way) based on people who
touched the same code, then it could very well be a common thing to
have 20 or more trailers on patches/commits for these projects.
There could also be automated testing tools that add their own
"Tested-by: *" trailers, and projects that require many people to add
their "Reviewed-by: *" trailers to each patch/commit. And in general
with millions of users, it is not very safe to make assumptions that
they will all be "reasonable" in the way they use a feature.
Another thing is that when Git started, I doubt many people would have
thought that there would often be more than just one or two
"Signed-off-by: *" trailer, and now 10 or 15 doesn't seem unthinkable.
In fact 'git interpret-trailers' has been made precisely because there
are more and more trailers, so there is more and more a need for a
tool to properly add and manage them.
We recently had a discussion on the list to increase the default
abbreviation length because it was not foreseen at the beginning of
Git that people would need a larger number of characters as projects
grow.
So even for the Linux kernel, I am not sure that it is safe to assume
that the number of trailers will not grow much over the years,
especially if we work on the tool that can make it easy to easily and
automatically add them.
Thanks,
Christian.
^ permalink raw reply
* Re: Formatting problem send_mail in version 2.10.0
From: Larry Finger @ 2016-10-12 15:27 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Jeff King, Mathieu Lienard--Mayor, Remi Lespinet, git
In-Reply-To: <vpq60oy589b.fsf@anie.imag.fr>
On 10/12/2016 02:36 AM, Matthieu Moy wrote:
> Larry Finger <Larry.Finger@lwfinger.net> writes:
>
>> On 10/11/2016 11:18 AM, Matthieu Moy wrote:
>>> Larry Finger <Larry.Finger@lwfinger.net> writes:
>>>
>>>> That added information at the end is intended to be passed on to the
>>>> stable group. In this case, the patch needs to be applied to kernel
>>>> versions 4.8 and later.
>>>
>>> OK, but where do people fetch this information from?
>>
>> This format is used in a patch for the kernel. When the patch is
>> merged into mainline, stable@vger.kernel.org gets sent an E-mail with
>> a copy of the original patch. Maintainers of the indicated systems
>> then merge the patch with their stable version.
>
> Sorry, but this does not answer my question. I'll rephrase: when
> people behind stable@vger.kernel.org get the message, how do they know
> which version of the kernel they should apply it to?
>
>> I do not want it in the body of the message. I just want to pass a
>> hint to the stable maintainer(s).
>
> If it's not in the body of the message, then where is it?
That information is supplied in the "Cc: Stable" line at the end. That is the
point for the "[4.8+]" notation. That means apply to kernel 4.8 and later.
Without this information, the patch author has to monitor the mainline kernel
for the application of the main patch, and then send a separate E-mail to Stable
indicating the version info. When the info is included, then the process is
automatic. I hope you understand now.
Larry
^ permalink raw reply
* problem with git worktree and git svn
From: Mathieu Arnold @ 2016-10-12 14:45 UTC (permalink / raw)
To: git
[-- Attachment #1.1: Type: text/plain, Size: 974 bytes --]
Hi,
I'm a FreeBSD ports developer, and while our main repository is in
Subversion, some of us use git-svn a lot for development.
I discovered git worktree earlier this week, and I found it a great
asset to be able to have more than one branch of my worktree accessible
at the same time...
Anyway, back to my problem, the way git-svn works, is that it looks for
a directory named "svn" in its gitdir and if it is not present, decide
the repository is using git-svn version 1 (whatever that is) and goes to
parse all the revisions to recreate the svn directory.
So I can only use git svn commands in my main worktree, the one with the
real gitdir.
To fix that, all I had to do is to add a symlink named svn in each
worktree's gitdir and pointing to ../../svn.
I think all that needs to happen is that when adding a new worktree, if
the main git directory has a "svn" directory, add a symlink to it in the
worktree's gitdir.
--
Mathieu Arnold
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 949 bytes --]
^ permalink raw reply
* Re: git branches & merge
From: Anatoly Borodin @ 2016-10-12 14:43 UTC (permalink / raw)
To: webmaster; +Cc: git
In-Reply-To: <1240187899.41938.1476280216470.JavaMail.open-xchange@app08.ox.hosteurope.de>
Hi,
the IP will not be overwritten, you'll still have the new IP in
master. Nothing to worry about :)
--
Mit freundlichen Grüßen,
Anatoly Borodin
^ permalink raw reply
* Re: [PATCH] sequencer: mark a file-local symbol static
From: Johannes Schindelin @ 2016-10-12 14:17 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, GIT Mailing-list
In-Reply-To: <376d8514-fe01-cc37-691d-6e101af564a5@ramsayjones.plus.com>
Hi Ramsay!
On Wed, 12 Oct 2016, Ramsay Jones wrote:
> If you need to re-roll your 'js/prepare-sequencer' branch, could you
> please squash this into commit 53f8024e ("sequencer: completely revamp
> the "todo" script parsing", 10-10-2016).
Will do!
Thanks,
Dscho
^ permalink raw reply
* Re: [PATCH 1/2] submodule: ignore trailing slash on superproject URL
From: Johannes Schindelin @ 2016-10-12 13:30 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, git, venv21, dennis
In-Reply-To: <20161010175611.1058-1-sbeller@google.com>
Hi Stefan,
On Mon, 10 Oct 2016, Stefan Beller wrote:
> Before 63e95beb0 (2016-04-15, submodule: port resolve_relative_url from
> shell to C), it did not matter if the superprojects URL had a trailing
> slash or not. It was just chopped off as one of the first steps
> (The "remoteurl=${remoteurl%/}" near the beginning of
> resolve_relative_url(), which was removed in said commit).
>
> When porting this to the C version, an off-by-one error was introduced
> and we did not check the actual last character to be a slash, but the
> NULL delimiter.
It is a NUL delimiter, not a NULL delimiter.
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 444ec06..a7841a5 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -95,6 +95,8 @@ static int chop_last_dir(char **remoteurl, int is_relative)
> * NEEDSWORK: This works incorrectly on the domain and protocol part.
> * remote_url url outcome expectation
> * http://a.com/b ../c http://a.com/c as is
> + * http://a.com/b/ ../c http://a.com/c same as previous line, but
> + * ignore trailing slash in url
> * http://a.com/b ../../c http://c error out
> * http://a.com/b ../../../c http:/c error out
> * http://a.com/b ../../../../c http:c error out
> @@ -113,8 +115,8 @@ static char *relative_url(const char *remote_url,
> struct strbuf sb = STRBUF_INIT;
> size_t len = strlen(remoteurl);
>
> - if (is_dir_sep(remoteurl[len]))
> - remoteurl[len] = '\0';
> + if (is_dir_sep(remoteurl[len-1]))
> + remoteurl[len-1] = '\0';
>
> if (!url_is_local_not_ssh(remoteurl) || is_absolute_path(remoteurl))
> is_relative = 0;
> diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
> index bf2deee..82b98f8 100755
> --- a/t/t0060-path-utils.sh
> +++ b/t/t0060-path-utils.sh
> @@ -319,6 +319,7 @@ test_submodule_relative_url "../" "foo/bar" "../submodule" "../foo/submodule"
> test_submodule_relative_url "../" "foo" "../submodule" "../submodule"
>
> test_submodule_relative_url "(null)" "../foo/bar" "../sub/a/b/c" "../foo/sub/a/b/c"
> +test_submodule_relative_url "(null)" "../foo/bar/" "../sub/a/b/c" "../foo/sub/a/b/c"
> test_submodule_relative_url "(null)" "../foo/bar" "../submodule" "../foo/submodule"
> test_submodule_relative_url "(null)" "../foo/submodule" "../submodule" "../foo/submodule"
> test_submodule_relative_url "(null)" "../foo" "../submodule" "../submodule"
I see that this already made it to `next`. I saw that because it breaks
the build of Git for Windows (this was not noticed earlier because other
compile failures prevented the tests from running), as now the test cases
173 and 177 of t0060 fail (*not* the newly introduced 163).
Here is the output with -v -x:
-- snip --
[...]
expecting success:
actual=$(git submodule--helper resolve-relative-url-test '(null)' '/usr/src/git/wip/t/trash directory.t0060-path-utils/.' '../.') &&
test "$actual" = 'C:/git-sdk-64/usr/src/git/wip/t/trash directory.t0060-path-utils/.'
+++ git submodule--helper resolve-relative-url-test '(null)' '/usr/src/git/wip/t/trash directory.t0060-path-utils/.' ../.
++ actual=C:/git-sdk-64/usr/src/git/wip/t/.
++ test C:/git-sdk-64/usr/src/git/wip/t/. = 'C:/git-sdk-64/usr/src/git/wip/t/trash directory.t0060-path-utils/.'
error: last command exited with $?=1
not ok 172 - test_submodule_relative_url: (null) /usr/src/git/wip/t/trash directory.t0060-path-utils/. ../. => C:/git-sdk-64/usr/src/git/wip/t/trash directory.t0060-path-utils/.
#
# actual=$(git submodule--helper
# resolve-relative-url-test '(null)'
# '/usr/src/git/wip/t/trash
# directory.t0060-path-utils/.' '../.') &&
# test "$actual" =
# 'C:/git-sdk-64/usr/src/git/wip/t/trash
# directory.t0060-path-utils/.'
#
[...]
expecting success:
actual=$(git submodule--helper resolve-relative-url-test '(null)' '/usr/src/git/wip/t/trash directory.t0060-path-utils/.' '../submodule') &&
test "$actual" = 'C:/git-sdk-64/usr/src/git/wip/t/trash directory.t0060-path-utils/submodule'
+++ git submodule--helper resolve-relative-url-test '(null)' '/usr/src/git/wip/t/trash directory.t0060-path-utils/.' ../submodule
++ actual=C:/git-sdk-64/usr/src/git/wip/t/submodule
++ test C:/git-sdk-64/usr/src/git/wip/t/submodule = 'C:/git-sdk-64/usr/src/git/wip/t/trash directory.t0060-path-utils/submodule'
error: last command exited with $?=1
not ok 176 - test_submodule_relative_url: (null) /usr/src/git/wip/t/trash directory.t0060-path-utils/. ../submodule => C:/git-sdk-64/usr/src/git/wip/t/trash directory.t0060-path-utils/submodule
#
# actual=$(git submodule--helper
# resolve-relative-url-test '(null)'
# '/usr/src/git/wip/t/trash
# directory.t0060-path-utils/.' '../submodule') &&
# test "$actual" =
# 'C:/git-sdk-64/usr/src/git/wip/t/trash
# directory.t0060-path-utils/submodule'
#
[...]
-- snap --
For comparison, this is how it succeeds in an Ubuntu VM:
-- snap --
expecting success:
actual=$(git submodule--helper resolve-relative-url-test '(null)' '/home/virtualbox/git/wip/t/trash directory.t0060-path-utils/.' '../.') &&
test "$actual" = '/home/virtualbox/git/wip/t/trash directory.t0060-path-utils/.'
+++ git submodule--helper resolve-relative-url-test '(null)' '/home/virtualbox/git/wip/t/trash directory.t0060-path-utils/.' ../.
++ actual='/home/virtualbox/git/wip/t/trash directory.t0060-path-utils/.'
++ test '/home/virtualbox/git/wip/t/trash directory.t0060-path-utils/.' = '/home/virtualbox/git/wip/t/trash directory.t0060-path-utils/.'
+ test_eval_ret_=0
+ want_trace
+ test t = t
+ test t = t
+ set +x
ok 173 - test_submodule_relative_url: (null) /home/virtualbox/git/wip/t/trash directory.t0060-path-utils/. ../. => /home/virtualbox/git/wip/t/trash directory.t0060-path-utils/.
-- snap --
The reason that this fails on Windows is that the POSIX->Windows path
mangling of the MSYS2 shell strips the trailing . from "/some/directory/."
when converting it to "C:/git-sdk-64/some/directory", and for a good
reason: most Windows programs do not handle the trailing "." very well.
One very, very ugly workaround for this newly-introduced breakage would be
this:
-- snip --
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 82b98f8..abd82e9 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -328,11 +328,11 @@ test_submodule_relative_url "(null)" "./foo" "../submodule" "submodule"
test_submodule_relative_url "(null)" "//somewhere else/repo" "../subrepo" "//somewhere else/subrepo"
test_submodule_relative_url "(null)" "$PWD/subsuper_update_r" "../subsubsuper_update_r" "$(pwd)/subsubsuper_update_r"
test_submodule_relative_url "(null)" "$PWD/super_update_r2" "../subsuper_update_r" "$(pwd)/subsuper_update_r"
-test_submodule_relative_url "(null)" "$PWD/." "../." "$(pwd)/."
+test_submodule_relative_url "(null)" "$(pwd)/." "../." "$(pwd)/."
test_submodule_relative_url "(null)" "$PWD" "./." "$(pwd)/."
test_submodule_relative_url "(null)" "$PWD/addtest" "../repo" "$(pwd)/repo"
test_submodule_relative_url "(null)" "$PWD" "./ " "$(pwd)/ "
-test_submodule_relative_url "(null)" "$PWD/." "../submodule" "$(pwd)/submodule"
+test_submodule_relative_url "(null)" "$(pwd)/." "../submodule" "$(pwd)/submodule"
test_submodule_relative_url "(null)" "$PWD/submodule" "../submodule" "$(pwd)/submodule"
test_submodule_relative_url "(null)" "$PWD/home2/../remote" "../bundle1" "$(pwd)/home2/../bundle1"
test_submodule_relative_url "(null)" "$PWD/submodule_update_repo" "./." "$(pwd)/submodule_update_repo/."
-- snap --
The reasons this is ugly: we specifically test for *Unixy* paths when we
use $PWD, as opposed to *Windowsy* paths when using $(pwd). We do this to
ensure a certain level of confidence that running things such as
git clone --recurse-submodules /z/project/.
work. And now that does not work anymore.
So where to go from here?
Ciao,
Dscho
^ permalink raw reply related
* git branches & merge
From: webmaster @ 2016-10-12 13:50 UTC (permalink / raw)
To: git
Hi.
I have created a release_x branch to work on a new release. The branch is based
on master.
Later I needed to create a hotfix_x branch to work on a hotfix. This branch is
also based on master.
In the hotfix_x branch I needed to correct an IP address to communicate with.
After doing so, I merged the hotfix_x branch with master again and made my
deployment.
Now in the release_x branche the IP address is still the old one.
I am afraid of committing and merging my release_x branch since I think that my
newly defined IP address (now on master) will be overridden when merging.
Am I right? If yes, what is the best way to solve this?
Thanks
-fuz
^ permalink raw reply
* Re: git diff
From: webmaster @ 2016-10-12 13:51 UTC (permalink / raw)
To: Mike Rappazzo; +Cc: git
In-Reply-To: <CANoM8SUtrV8CD8M=4CUB8xr9gQj_hKzGOdjQZHuz4+36y_UfJw@mail.gmail.com>
Thanks, Mike. Now I understood.
-fuz
> Mike Rappazzo <rappazzo@gmail.com> hat am 12. Oktober 2016 um 13:06
> geschrieben:
>
> On Wed, Oct 12, 2016 at 6:50 AM, <webmaster@peter-speer.de> wrote:
> > Hi.
> >
> > I created a new branch named hotfix from master.
> > I switched to the branch, changed 1 file.
> >
> > Now I want to see the diff from the both using
> >
> > git diff hotfix master
> >
> > I do not see any output (difference).
> > When I do a git status I see my file with status mofified, not staged for
> > commit.
>
> Since you just created the branch, and did not add any content, there
> is no difference to see. A branch is just a pointer to a commit. You
> now have two pointers pointing at the same commit.
>
> If you want to see the difference between your changes and the master
> branch, you can omit the first reference:
>
> git diff master
>
> When you start adding commits to your hotfix branch, you will be able
> to see the diff between that and master with the command that you
> gave. However, your arguments may be in the reverse order than what
> you expect. You want to specify master first because that is the
> mainline branch (I presume).
>
> When you have several commits on your hotfix branch, you can refer to
> older commits to diff against. There are several ways to refer back,
> but the simplest is to use a tilde '~' followed by a number to count
> back. For example 'hotfix~1' refers to the parent commit on the
> hotfix branch. There is a lot in the documentation[1], so take a look
> there for more info.
>
> Good luck.
> _Mike
>
> [1] https://git-scm.com/doc
>
> > Also, I can see that I am working with the correct branch, hotfix
> >
> > What am I doing wrong?
> >
> > -fuz
>
> On Wed, Oct 12, 2016 at 6:50 AM, <webmaster@peter-speer.de> wrote:
> > Hi.
> >
> > I created a new branch named hotfix from master.
> > I switched to the branch, changed 1 file.
> >
> > Now I want to see the diff from the both using
> >
> > git diff hotfix master
> >
> > I do not see any output (difference).
> > When I do a git status I see my file with status mofified, not staged for
> > commit.
> > Also, I can see that I am working with the correct branch, hotfix
> >
> > What am I doing wrong?
> >
> > -fuz
>
^ permalink raw reply
* [PATCH v2 2/2] convert.c: stream and fast search for binary
From: tboegi @ 2016-10-12 13:47 UTC (permalink / raw)
To: git; +Cc: Torsten Bögershausen
In-Reply-To: <20161009095649.1886-1-tboegi@web.de>
From: Torsten Bögershausen <tboegi@web.de>
When statistics are done for the autocrlf handling, the search in
the content can be stopped, if e.g
- a search for binary is done, and a NUL character is found
- a search for CRLF is done, and the first CRLF is found.
Similar when statistics for binary vs non-binary are gathered:
Whenever a lone CR or NUL is found, the search can be aborted.
When checking out files in "auto" mode, any file that has a "lone CR"
or a CRLF will not be converted, so the search can be aborted early.
Add the new bit, CONVERT_STAT_BITS_ANY_CR,
which is set for either lone CR or CRLF.
Many binary files have a NUL very early and it is often not necessary
to load the whole content of a file or blob into memory.
Split gather_stats() into gather_all_stats() and gather_stats_partly()
to do a streaming handling for blobs and files in the worktree.
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
convert.c | 191 ++++++++++++++++++++++++++++++++++++++++++--------------------
1 file changed, 129 insertions(+), 62 deletions(-)
diff --git a/convert.c b/convert.c
index 077f5e6..2396fe5 100644
--- a/convert.c
+++ b/convert.c
@@ -3,6 +3,7 @@
#include "run-command.h"
#include "quote.h"
#include "sigchain.h"
+#include "streaming.h"
/*
* convert.c - convert a file when checking it out and checking it in.
@@ -13,10 +14,12 @@
* translation when the "text" attribute or "auto_crlf" option is set.
*/
-/* Stat bits: When BIN is set, the txt bits are unset */
#define CONVERT_STAT_BITS_TXT_LF 0x1
#define CONVERT_STAT_BITS_TXT_CRLF 0x2
#define CONVERT_STAT_BITS_BIN 0x4
+#define CONVERT_STAT_BITS_ANY_CR 0x8
+
+#define STREAM_BUFFER_SIZE (1024*16)
enum crlf_action {
CRLF_UNDEFINED,
@@ -31,30 +34,36 @@ enum crlf_action {
struct text_stat {
/* NUL, CR, LF and CRLF counts */
- unsigned nul, lonecr, lonelf, crlf;
+ unsigned stat_bits, lonecr, lonelf, crlf;
/* These are just approximations! */
unsigned printable, nonprintable;
};
-static void gather_stats(const char *buf, unsigned long size, struct text_stat *stats)
+static void gather_stats_partly(const char *buf, unsigned long size,
+ struct text_stat *stats, unsigned search_only)
{
unsigned long i;
- memset(stats, 0, sizeof(*stats));
-
+ if (!buf || !size)
+ return;
for (i = 0; i < size; i++) {
unsigned char c = buf[i];
if (c == '\r') {
+ stats->stat_bits |= CONVERT_STAT_BITS_ANY_CR;
if (i+1 < size && buf[i+1] == '\n') {
stats->crlf++;
i++;
- } else
+ stats->stat_bits |= CONVERT_STAT_BITS_TXT_CRLF;
+ } else {
stats->lonecr++;
+ stats->stat_bits |= CONVERT_STAT_BITS_BIN;
+ }
continue;
}
if (c == '\n') {
stats->lonelf++;
+ stats->stat_bits |= CONVERT_STAT_BITS_TXT_LF;
continue;
}
if (c == 127)
@@ -67,7 +76,7 @@ static void gather_stats(const char *buf, unsigned long size, struct text_stat *
stats->printable++;
break;
case 0:
- stats->nul++;
+ stats->stat_bits |= CONVERT_STAT_BITS_BIN;
/* fall through */
default:
stats->nonprintable++;
@@ -75,6 +84,8 @@ static void gather_stats(const char *buf, unsigned long size, struct text_stat *
}
else
stats->printable++;
+ if (stats->stat_bits & search_only)
+ break; /* We found what we have been searching for */
}
/* If file ends with EOF then don't count this EOF as non-printable. */
@@ -86,41 +97,62 @@ static void gather_stats(const char *buf, unsigned long size, struct text_stat *
* The same heuristics as diff.c::mmfile_is_binary()
* We treat files with bare CR as binary
*/
-static int convert_is_binary(unsigned long size, const struct text_stat *stats)
+static void convert_nonprintable(struct text_stat *stats)
{
- if (stats->lonecr)
- return 1;
- if (stats->nul)
- return 1;
if ((stats->printable >> 7) < stats->nonprintable)
- return 1;
- return 0;
+ stats->stat_bits |= CONVERT_STAT_BITS_BIN;
}
-static unsigned int gather_convert_stats(const char *data, unsigned long size)
+static void gather_all_stats(const char *buf, unsigned long size,
+ struct text_stat *stats, unsigned search_only)
{
+ memset(stats, 0, sizeof(*stats));
+ gather_stats_partly(buf, size, stats, search_only);
+ convert_nonprintable(stats);
+}
+
+
+static unsigned get_convert_stats_sha1(unsigned const char *sha1,
+ unsigned search_only)
+{
+ struct git_istream *st;
struct text_stat stats;
- int ret = 0;
- if (!data || !size)
- return 0;
- gather_stats(data, size, &stats);
- if (convert_is_binary(size, &stats))
- ret |= CONVERT_STAT_BITS_BIN;
- if (stats.crlf)
- ret |= CONVERT_STAT_BITS_TXT_CRLF;
- if (stats.lonelf)
- ret |= CONVERT_STAT_BITS_TXT_LF;
+ enum object_type type;
+ unsigned long sz;
- return ret;
+ if (!sha1)
+ return 0;
+ memset(&stats, 0, sizeof(stats));
+ st = open_istream(sha1, &type, &sz, NULL);
+ if (!st) {
+ return 0;
+ }
+ if (type != OBJ_BLOB)
+ goto close_and_exit_i;
+ for (;;) {
+ char buf[STREAM_BUFFER_SIZE];
+ ssize_t readlen = read_istream(st, buf, sizeof(buf));
+ if (readlen < 0)
+ break;
+ if (!readlen)
+ break;
+ gather_stats_partly(buf, (unsigned long)readlen, &stats, search_only);
+ if (stats.stat_bits & search_only)
+ break; /* We found what we have been searching for */
+ }
+close_and_exit_i:
+ close_istream(st);
+ convert_nonprintable(&stats);
+ return stats.stat_bits;
}
-static const char *gather_convert_stats_ascii(const char *data, unsigned long size)
+static const char *convert_stats_ascii(unsigned convert_stats)
{
- unsigned int convert_stats = gather_convert_stats(data, size);
-
+ const unsigned eol_bits = CONVERT_STAT_BITS_TXT_LF |
+ CONVERT_STAT_BITS_TXT_CRLF;
if (convert_stats & CONVERT_STAT_BITS_BIN)
return "-text";
- switch (convert_stats) {
+ switch (convert_stats & eol_bits) {
case CONVERT_STAT_BITS_TXT_LF:
return "lf";
case CONVERT_STAT_BITS_TXT_CRLF:
@@ -132,24 +164,45 @@ static const char *gather_convert_stats_ascii(const char *data, unsigned long si
}
}
+static unsigned get_convert_stats_wt(const char *path)
+{
+ struct text_stat stats;
+ unsigned search_only = CONVERT_STAT_BITS_BIN;
+ int fd;
+ memset(&stats, 0, sizeof(stats));
+ fd = open(path, O_RDONLY);
+ if (fd < 0)
+ return 0;
+ for (;;) {
+ char buf[STREAM_BUFFER_SIZE];
+ ssize_t readlen = read(fd, buf, sizeof(buf));
+ if (readlen < 0)
+ break;
+ if (!readlen)
+ break;
+ gather_stats_partly(buf, (unsigned long)readlen, &stats, search_only);
+ if (stats.stat_bits & search_only)
+ break; /* We found what we have been searching for */
+ }
+ close(fd);
+ convert_nonprintable(&stats);
+ return stats.stat_bits;
+}
+
const char *get_cached_convert_stats_ascii(const char *path)
{
- const char *ret;
- unsigned long sz;
- void *data = read_blob_data_from_cache(path, &sz);
- ret = gather_convert_stats_ascii(data, sz);
- free(data);
- return ret;
+ unsigned convert_stats;
+ unsigned search_only = CONVERT_STAT_BITS_BIN;
+ convert_stats = get_convert_stats_sha1(get_sha1_from_cache(path),
+ search_only);
+ return convert_stats_ascii(convert_stats);
}
const char *get_wt_convert_stats_ascii(const char *path)
{
- const char *ret = "";
- struct strbuf sb = STRBUF_INIT;
- if (strbuf_read_file(&sb, path, 0) >= 0)
- ret = gather_convert_stats_ascii(sb.buf, sb.len);
- strbuf_release(&sb);
- return ret;
+ unsigned convert_stats;
+ convert_stats = get_convert_stats_wt(path);
+ return convert_stats_ascii(convert_stats);
}
static int text_eol_is_crlf(void)
@@ -213,16 +266,10 @@ static void check_safe_crlf(const char *path, enum crlf_action crlf_action,
static int has_cr_in_index(const char *path)
{
- unsigned long sz;
- void *data;
- int has_cr;
-
- data = read_blob_data_from_cache(path, &sz);
- if (!data)
- return 0;
- has_cr = memchr(data, '\r', sz) != NULL;
- free(data);
- return has_cr;
+ unsigned convert_stats;
+ convert_stats = get_convert_stats_sha1(get_sha1_from_cache(path),
+ CONVERT_STAT_BITS_ANY_CR);
+ return convert_stats & CONVERT_STAT_BITS_ANY_CR;
}
static int will_convert_lf_to_crlf(size_t len, struct text_stat *stats,
@@ -234,13 +281,13 @@ static int will_convert_lf_to_crlf(size_t len, struct text_stat *stats,
if (!stats->lonelf)
return 0;
- if (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_INPUT || crlf_action == CRLF_AUTO_CRLF) {
+ if (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_CRLF) {
/* If we have any CR or CRLF line endings, we do not touch it */
/* This is the new safer autocrlf-handling */
if (stats->lonecr || stats->crlf)
return 0;
- if (convert_is_binary(len, stats))
+ if (stats->stat_bits & CONVERT_STAT_BITS_BIN)
return 0;
}
return 1;
@@ -253,7 +300,8 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
{
struct text_stat stats;
char *dst;
- int convert_crlf_into_lf;
+ int has_crlf_to_convert;
+ unsigned search_only = 0;
if (crlf_action == CRLF_BINARY ||
(src && !len))
@@ -266,12 +314,16 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
if (!buf && !src)
return 1;
- gather_stats(src, len, &stats);
+ if (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_INPUT || crlf_action == CRLF_AUTO_CRLF)
+ search_only = CONVERT_STAT_BITS_BIN;
+
+ gather_all_stats(src, len, &stats, search_only);
+
/* Optimization: No CRLF? Nothing to convert, regardless. */
- convert_crlf_into_lf = !!stats.crlf;
+ has_crlf_to_convert = !!stats.crlf;
if (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_INPUT || crlf_action == CRLF_AUTO_CRLF) {
- if (convert_is_binary(len, &stats))
+ if (stats.stat_bits & CONVERT_STAT_BITS_BIN)
return 0;
/*
* If the file in the index has any CR in it, do not convert.
@@ -280,24 +332,35 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
if (checksafe == SAFE_CRLF_RENORMALIZE)
checksafe = SAFE_CRLF_FALSE;
else if (has_cr_in_index(path))
- convert_crlf_into_lf = 0;
+ has_crlf_to_convert = 0;
}
if (checksafe && len) {
struct text_stat new_stats;
memcpy(&new_stats, &stats, sizeof(new_stats));
/* simulate "git add" */
- if (convert_crlf_into_lf) {
+ if (has_crlf_to_convert) {
new_stats.lonelf += new_stats.crlf;
new_stats.crlf = 0;
+ /* all crlf, if any, are gone. Update the bits */
+ new_stats.stat_bits = stats.stat_bits & CONVERT_STAT_BITS_BIN;
+ if (new_stats.lonelf)
+ new_stats.stat_bits |= CONVERT_STAT_BITS_TXT_LF;
+ if (new_stats.lonecr)
+ new_stats.stat_bits |= CONVERT_STAT_BITS_ANY_CR;
}
/* simulate "git checkout" */
if (will_convert_lf_to_crlf(len, &new_stats, crlf_action)) {
new_stats.crlf += new_stats.lonelf;
new_stats.lonelf = 0;
+ new_stats.stat_bits = stats.stat_bits & CONVERT_STAT_BITS_BIN;
+ if (new_stats.crlf)
+ new_stats.stat_bits |= CONVERT_STAT_BITS_TXT_CRLF | CONVERT_STAT_BITS_ANY_CR;
+ if (new_stats.lonecr)
+ new_stats.stat_bits |= CONVERT_STAT_BITS_ANY_CR;
}
check_safe_crlf(path, crlf_action, &stats, &new_stats, checksafe);
}
- if (!convert_crlf_into_lf)
+ if (!has_crlf_to_convert)
return 0;
/*
@@ -338,11 +401,15 @@ static int crlf_to_worktree(const char *path, const char *src, size_t len,
{
char *to_free = NULL;
struct text_stat stats;
+ unsigned search_only = 0;
if (!len || output_eol(crlf_action) != EOL_CRLF)
return 0;
- gather_stats(src, len, &stats);
+ if (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_CRLF)
+ search_only = CONVERT_STAT_BITS_ANY_CR | CONVERT_STAT_BITS_BIN;
+
+ gather_all_stats(src, len, &stats, search_only);
if (!will_convert_lf_to_crlf(len, &stats, crlf_action))
return 0;
--
2.10.0
^ permalink raw reply related
* [PATCH v2 0/2] Stream and fast search
From: tboegi @ 2016-10-12 13:47 UTC (permalink / raw)
To: git; +Cc: Torsten Bögershausen
In-Reply-To: <20161009095649.1886-1-tboegi@web.de>
From: Torsten Bögershausen <tboegi@web.de>
Changes since v1:
- Rename earlyout into search_only
- Increase buffer from 2KiB to 16KiB
- s/mask/eol_bits/
- Reduce the "noise"
- Document "split gather_stats() into gather_all_stats()/gather_stats_partly()
Torsten Bögershausen (2):
read-cache: factor out get_sha1_from_index() helper
convert.c: stream and fast search for binary
cache.h | 3 +
convert.c | 191 ++++++++++++++++++++++++++++++++++++++++-------------------
read-cache.c | 29 +++++----
3 files changed, 150 insertions(+), 73 deletions(-)
--
2.10.0
^ permalink raw reply
* [PATCH v2 1/2] read-cache: factor out get_sha1_from_index() helper
From: tboegi @ 2016-10-12 13:47 UTC (permalink / raw)
To: git; +Cc: Torsten Bögershausen
In-Reply-To: <20161009095649.1886-1-tboegi@web.de>
From: Torsten Bögershausen <tboegi@web.de>
Factor out the retrieval of the sha1 for a given path in
read_blob_data_from_index() into the function get_sha1_from_index().
This will be used in the next commit, when convert.c can do the
analyze for "text=auto" without slurping the whole blob into memory
at once.
Add a wrapper definition get_sha1_from_cache().
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
cache.h | 3 +++
read-cache.c | 29 ++++++++++++++++++-----------
2 files changed, 21 insertions(+), 11 deletions(-)
diff --git a/cache.h b/cache.h
index 1604e29..04de209 100644
--- a/cache.h
+++ b/cache.h
@@ -380,6 +380,7 @@ extern void free_name_hash(struct index_state *istate);
#define unmerge_cache_entry_at(at) unmerge_index_entry_at(&the_index, at)
#define unmerge_cache(pathspec) unmerge_index(&the_index, pathspec)
#define read_blob_data_from_cache(path, sz) read_blob_data_from_index(&the_index, (path), (sz))
+#define get_sha1_from_cache(path) get_sha1_from_index (&the_index, (path))
#endif
enum object_type {
@@ -1089,6 +1090,8 @@ static inline void *read_sha1_file(const unsigned char *sha1, enum object_type *
return read_sha1_file_extended(sha1, type, size, LOOKUP_REPLACE_OBJECT);
}
+const unsigned char *get_sha1_from_index(struct index_state *istate, const char *path);
+
/*
* This internal function is only declared here for the benefit of
* lookup_replace_object(). Please do not call it directly.
diff --git a/read-cache.c b/read-cache.c
index 38d67fa..5a1df14 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2290,13 +2290,27 @@ int index_name_is_other(const struct index_state *istate, const char *name,
void *read_blob_data_from_index(struct index_state *istate, const char *path, unsigned long *size)
{
- int pos, len;
+ const unsigned char *sha1;
unsigned long sz;
enum object_type type;
void *data;
- len = strlen(path);
- pos = index_name_pos(istate, path, len);
+ sha1 = get_sha1_from_index(istate, path);
+ if (!sha1)
+ return NULL;
+ data = read_sha1_file(sha1, &type, &sz);
+ if (!data || type != OBJ_BLOB) {
+ free(data);
+ return NULL;
+ }
+ if (size)
+ *size = sz;
+ return data;
+}
+
+const unsigned char *get_sha1_from_index(struct index_state *istate, const char *path)
+{
+ int pos = index_name_pos(istate, path, strlen(path));
if (pos < 0) {
/*
* We might be in the middle of a merge, in which
@@ -2312,14 +2326,7 @@ void *read_blob_data_from_index(struct index_state *istate, const char *path, un
}
if (pos < 0)
return NULL;
- data = read_sha1_file(istate->cache[pos]->oid.hash, &type, &sz);
- if (!data || type != OBJ_BLOB) {
- free(data);
- return NULL;
- }
- if (size)
- *size = sz;
- return data;
+ return istate->cache[pos]->oid.hash;
}
void stat_validity_clear(struct stat_validity *sv)
--
2.10.0
^ permalink raw reply related
* Re: [PATCH v2 3/3] batch check whether submodule needs pushing into one call
From: Heiko Voigt @ 2016-10-12 13:33 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Stefan Beller, git, Jens.Lehmann, Fredrik Gustafsson,
Leandro Lucarella
In-Reply-To: <xmqqlgxvbype.fsf@gitster.mtv.corp.google.com>
On Mon, Oct 10, 2016 at 03:56:13PM -0700, Junio C Hamano wrote:
> Heiko Voigt <hvoigt@hvoigt.net> writes:
>
> > -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;
>
> Same comment about naming.
>
> What do check-has-hash and submodule-has-hashes exactly mean by
> "hash" in their names? Because I think what is checked here is
> "does the local submodule repository have _all_ the commits
> referenced from the superproject commit we are pushing?", so I'd
> prefer to see "commit" in their names.
>
> If we do not even have these commits locally, then there is no point
> attempting to push, so returning 0 (i.e. it is not "needs pushing"
> situation) is correct but it is a but subtle. It's not "we know
> they already have them", but it is "even if we tried to push, it
> won't do us or the other side any good." A single-liner in-code
> comment may help.
First the naming part. How about:
submodule_has_commits()
?
Second as mentioned a previous answer[1] to this part: I would actually
like to have a die() here instead of blindly proceeding. Since the user
either specified --recurse-submodules=... at the commandline or it was
implicitly enabled because we have submodules in the tree we should be
careful and not push revisions referencing submodules that are not
available at a remote. If we can not properly figure it out I would
suggest to stop and tell the user how to solve the situation. E.g.
either she clones the appropriate submodules or specifies
--no-recurse-submodules on the commandline to tell git that she does not
care.
Returning 0 here means: "No push needed" but the correct answer would
be: "We do not know". Question is what we should do here which I am
planning to address in a separate patch series since that will be
changing behavior.
So how about:
if (!submodule_has_hashes(path, hashes))
/* NEEDSWORK: The correct answer here is "We do not
* know" instead of "No". We currently proceed pushing
* here as if the submodules commits are available on a
* remote, which is not always correct. */
return 0;
What do you think?
Cheers Heiko
[1] http://public-inbox.org/git/20160919195812.GC62429@book.hvoigt.net/
^ permalink raw reply
* Re: interactive rebase should better highlight the not-applying commit
From: Joshua N Pritikin @ 2016-10-12 13:27 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org
In-Reply-To: <CAGZ79kZSQx7aOCgQ2dwzJeCLX-k-+x1SKabEBG7CktNfeXAbvg@mail.gmail.com>
On Tue, Oct 11, 2016 at 01:55:22PM -0700, Stefan Beller wrote:
> On Tue, Oct 11, 2016 at 12:07 PM, Joshua N Pritikin <jpritikin@pobox.com> wrote:
> > I assume somebody familiar with GIT's code base could make this change
> > in about 10 minutes.
>
> Can you elaborate how you come to that estimate?
Hm, a false belief in the general awesomeness of GIT developers?
On Tue, Oct 11, 2016 at 02:25:19PM -0700, Stefan Beller wrote:
> On Tue, Oct 11, 2016 at 12:07 PM, Joshua N Pritikin <jpritikin@pobox.com> wrote:
> > As of GIT 2.8.1, if you do an interactive rebase and get some conflict
> > in the stack of patches then the commit with the conflict is buried in
> > 4-5 lines of output. It is visually difficult to immediately pick out
> > which commit did not apply cleanly. I suggest highlighting the 1 line
> > commit summary in red or green or some color to help it stand out from
> > all the other output.
> >
> > I decided to suggest this change after I realized that I probably
> > skipped a commit during an interactive rebase instead of resolving the
> > conflict. I knew I had to skip some commit so I assumed that I just need
> > to skip without reading the commit summary carefully. Now it is 7-15
> > days after I did the erroneous rebase. I had to spend a few hours today
> > with GIT's archaeology tools to find the lost code.
>
> Looking at the actual code, this is not as easy as one might assume,
> because rebase is written in shell. (One of the last remaining large
> commands in shell), and there is no color support in the die(..)
> function.
I'm sorry to hear that.
> However IIUC currently rebase is completely rewritten/ported to C
> where it is easier to add color support as we do have some color
> support in there already.
Sounds great. Is there a beta release that I can try out?
Also, I have another wishlist item for (interactive) rebase. Sometimes I
do a rebase to fix some tiny thing 10-15 commits from HEAD. Maybe only 1
file is affected and there are no merge conflicts, but when rebase
reapplies all the commits, the timestamps of lots of unmodified files
change even though they are unmodified compared to before the rebase.
Since the modification times are used by 'make' to compute dependencies,
this creates a lot of useless recompilation that slows things down. It
would be great if rebase only changed the timestamps of files that were
actually modified.
Thank you.
--
Joshua N. Pritikin, Ph.D.
Virginia Institute for Psychiatric and Behavioral Genetics
Virginia Commonwealth University
PO Box 980126
800 E Leigh St, Biotech One, Suite 1-133
Richmond, VA 23219
http://people.virginia.edu/~jnp3bc
^ permalink raw reply
* Re: [PATCH v2 1/3] serialize collection of changed submodules
From: Heiko Voigt @ 2016-10-12 13:11 UTC (permalink / raw)
To: Stefan Beller
Cc: Junio C Hamano, Jeff King, git@vger.kernel.org, Jens Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <CAGZ79kZiY56-84aThH1F02E_HzCTAK3KSYLbyP1D5GUAt892cw@mail.gmail.com>
On Fri, Oct 07, 2016 at 10:59:29AM -0700, Stefan Beller wrote:
> On Fri, Oct 7, 2016 at 8:06 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> > +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];
>
> You do not seem to make use of `i` explicitely, so
> for_each_string_list_item might be more readable here?
Will change.
> > @@ -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];
>
> You do not seem to make use of `i` explicitely, so
> for_each_string_list_item might be more readable here?
As above.
Cheers Heiko
^ permalink raw reply
* Re: [PATCH v2 2/3] serialize collection of refs that contain submodule changes
From: Heiko Voigt @ 2016-10-12 13:10 UTC (permalink / raw)
To: Stefan Beller
Cc: Junio C Hamano, Jeff King, git@vger.kernel.org, Jens Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <CAGZ79kZ5CPTuW2fARDs3BUt89b8H_=P3otZv+Vm5nTV70NLWtg@mail.gmail.com>
On Fri, Oct 07, 2016 at 11:16:31AM -0700, Stefan Beller wrote:
> > 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));
>
> Nit of the day:
> When using the struct child-process, we have the oldstyle argv NULL
> terminated array as
> well as the new style args argv_array. So in that context we'd prefer
> `args` as a name for
> argv_array as that helps to distinguish from the old array type.
> Here however `argv` seems to be a reasonable name, in fact whenever we
> do not deal with
> child processes, we seem to not like the `args` name:
>
> $ git grep argv_array |wc -l
> 577
> $ git grep argv_array |grep args |wc -l
> 293
>
> The rest looks good to me. :)
Thanks. So I do not completely get what you are suggesting: args or kept
it the way it is? Since in the end you are saying it is ok here ;) I
mainly chose this name because I am substituting the argv variable which
is already called 'argv' with this array. That might also be the reason
why in so many locations with struct child_processe's we have the 'argv'
name: Because they initially started with the old-style NULL terminated
array.
I am fine with it either way. Just tell me what you like :)
Cheers Heiko
^ permalink raw reply
* [PATCH v3] gpg-interface: use more status letters
From: Michael J Gruber @ 2016-10-12 13:04 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Alex
In-Reply-To: <xmqqtwckf5mg.fsf@gitster.mtv.corp.google.com>
According to gpg2's doc/DETAILS:
For each signature only one of the codes GOODSIG, BADSIG,
EXPSIG, EXPKEYSIG, REVKEYSIG or ERRSIG will be emitted.
gpg1 ("classic") behaves the same (although doc/DETAILS differs).
Currently, we parse gpg's status output for GOODSIG, BADSIG and
trust information and translate that into status codes G, B, U, N
for the %G? format specifier.
git-verify-* returns success in the GOODSIG case only. This is
somewhat in disagreement with gpg, which considers the first 5 of
the 6 above as VALIDSIG, but we err on the very safe side.
Introduce additional status codes E, X, Y, R for ERRSIG, EXPSIG,
EXPKEYSIG, and REVKEYSIG so that a user of %G? gets more information
about the absence of a 'G' on first glance.
Requested-by: Alex <agrambot@gmail.com>
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
v3 incorporates Junios' changes to the commit message, as well as his
suggestion how to use an undefined gpghome the way test-lib does.
Also, all Y-related changes (including the if in pretty.c).
Testing X, Y, and R from our test scripts is somewhat problematic
(some gpg versions do not allow back-dating, and we cannot ship pre-made
signatures easily) but I have tested all of them locally.
Documentation/pretty-formats.txt | 10 ++++++++--
gpg-interface.c | 13 ++++++++++---
pretty.c | 4 ++++
t/t7510-signed-commit.sh | 13 ++++++++++++-
4 files changed, 34 insertions(+), 6 deletions(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index a942d57f73..179c9389aa 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -143,8 +143,14 @@ ifndef::git-rev-list[]
- '%N': commit notes
endif::git-rev-list[]
- '%GG': raw verification message from GPG for a signed commit
-- '%G?': show "G" for a good (valid) signature, "B" for a bad signature,
- "U" for a good signature with unknown validity and "N" for no signature
+- '%G?': show "G" for a good (valid) signature,
+ "B" for a bad signature,
+ "U" for a good signature with unknown validity,
+ "X" for a good signature that has expired,
+ "Y" for a good signature made by an expired key,
+ "R" for a good signature made by a revoked key,
+ "E" if the signature cannot be checked (e.g. missing key)
+ and "N" for no signature
- '%GS': show the name of the signer for a signed commit
- '%GK': show the key used to sign a signed commit
- '%gD': reflog selector, e.g., `refs/stash@{1}` or
diff --git a/gpg-interface.c b/gpg-interface.c
index 8672edaf48..e44cc27da1 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -33,6 +33,10 @@ static struct {
{ 'B', "\n[GNUPG:] BADSIG " },
{ 'U', "\n[GNUPG:] TRUST_NEVER" },
{ 'U', "\n[GNUPG:] TRUST_UNDEFINED" },
+ { 'E', "\n[GNUPG:] ERRSIG "},
+ { 'X', "\n[GNUPG:] EXPSIG "},
+ { 'Y', "\n[GNUPG:] EXPKEYSIG "},
+ { 'R', "\n[GNUPG:] REVKEYSIG "},
};
void parse_gpg_output(struct signature_check *sigc)
@@ -54,9 +58,12 @@ void parse_gpg_output(struct signature_check *sigc)
/* The trust messages are not followed by key/signer information */
if (sigc->result != 'U') {
sigc->key = xmemdupz(found, 16);
- found += 17;
- next = strchrnul(found, '\n');
- sigc->signer = xmemdupz(found, next - found);
+ /* The ERRSIG message is not followed by signer information */
+ if (sigc-> result != 'E') {
+ found += 17;
+ next = strchrnul(found, '\n');
+ sigc->signer = xmemdupz(found, next - found);
+ }
}
}
}
diff --git a/pretty.c b/pretty.c
index 25efbcac92..d89ca30911 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1232,8 +1232,12 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
switch (c->signature_check.result) {
case 'G':
case 'B':
+ case 'E':
case 'U':
case 'N':
+ case 'X':
+ case 'Y':
+ case 'R':
strbuf_addch(sb, c->signature_check.result);
}
break;
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 6e839f5489..762135adea 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -2,6 +2,7 @@
test_description='signed commit tests'
. ./test-lib.sh
+GNUPGHOME_NOT_USED=$GNUPGHOME
. "$TEST_DIRECTORY/lib-gpg.sh"
test_expect_success GPG 'create signed commits' '
@@ -190,7 +191,7 @@ test_expect_success GPG 'show bad signature with custom format' '
test_cmp expect actual
'
-test_expect_success GPG 'show unknown signature with custom format' '
+test_expect_success GPG 'show untrusted signature with custom format' '
cat >expect <<-\EOF &&
U
61092E85B7227189
@@ -200,6 +201,16 @@ test_expect_success GPG 'show unknown signature with custom format' '
test_cmp expect actual
'
+test_expect_success GPG 'show unknown signature with custom format' '
+ cat >expect <<-\EOF &&
+ E
+ 61092E85B7227189
+
+ EOF
+ GNUPGHOME="$GNUPGHOME_NOT_USED" git log -1 --format="%G?%n%GK%n%GS" eighth-signed-alt >actual &&
+ test_cmp expect actual
+'
+
test_expect_success GPG 'show lack of signature with custom format' '
cat >expect <<-\EOF &&
N
--
2.10.1.532.gfe29b57
^ permalink raw reply related
* Re: [PATCH v2 1/3] serialize collection of changed submodules
From: Heiko Voigt @ 2016-10-12 13:00 UTC (permalink / raw)
To: Junio C Hamano
Cc: Stefan Beller, Jeff King, git@vger.kernel.org, Jens Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <xmqqvawzbzb2.fsf@gitster.mtv.corp.google.com>
On Mon, Oct 10, 2016 at 03:43:13PM -0700, Junio C Hamano wrote:
> Stefan Beller <sbeller@google.com> writes:
>
> >> +static struct sha1_array *get_sha1s_from_list(struct string_list *submodules,
> >> + const char *path)
> >
> > So this will take the stringlist `submodules` and insert the path into it,
> > if it wasn't already in there. In case it is newly inserted, add a sha1_array
> > as util, so each inserted path has it's own empty array.
> >
> > So it is both init of the data structures as well as retrieving them. I was
> > initially confused by the name as I assumed it would give you sha1s out
> > of a string list (e.g. transform strings to internal sha1 things).
> > Maybe it's just
> > me having a hard time to understand that, but I feel like the name could be
> > improved.
> >
> > lookup_sha1_list_by_path,
> > insert_path_and_return_sha1_list ?
>
> I do not think either the name or the "find if exists otherwise
> initialize one" behaviour is particularly confusing, but I do not
> think "maintain a set of sha1_arrays keyed with a string" is a so
> widely reusable general concept/construct. As can be seen easily in
> the names of parameters, this function is about maintaining a set of
> sha1_arrays keyed by paths to submodules, and I also assume that the
> array indexed by path is not meant to be a general purpose "we can
> use it to store any 40-hex thing" but to store something specific.
>
> What is that specific thing? The names of commit objects in the
> submodule repository?
>
> I'd prefer to see that exact thing used to construct the function
> name for a helper function with specific usage in mind, i.e.
> get_commit_object_names_for_submodule_path() or something along that
> line.
I did not name this function too precisely to keep it's name short since
everything specific was quite long, like the suggestion from Junio.
Since this is a static function local to the submodule file I was
assuming anyone interested would just look up the usage and immediately
see the purpose. If I look into submodule-cache.c where I have a similar
functionality we used 'lookup_or_create' for this create on demand
functionality. So a function name would be:
lookup_or_create_commit_objects_for_submodule_path(...
Which seems quite extensively long for a static function so how about
we shorten it a bit and add a comment:
/* lookup or create commit object list for submodule */
get_commit_objects_for_submodule_path(...
?
Cheers Heiko
^ permalink raw reply
* Re: [PATCH 2/2] reset: support the --stdin option
From: Johannes Schindelin @ 2016-10-12 12:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqeg3mai1b.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 11 Oct 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > + if (read_from_stdin) {
> > + strbuf_getline_fn getline_fn = nul_term_line ?
> > + strbuf_getline_nul : strbuf_getline_lf;
> > + int flags = PATHSPEC_PREFER_FULL |
> > + PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP;
> > + struct strbuf buf = STRBUF_INIT;
> > + struct strbuf unquoted = STRBUF_INIT;
> > +
> > + if (patch_mode)
> > + die(_("--stdin is incompatible with --patch"));
> > +
> > + if (pathspec.nr)
> > + die(_("--stdin is incompatible with path arguments"));
> > +
> > + if (patch_mode)
> > + flags |= PATHSPEC_PREFIX_ORIGIN;
>
> Didn't we already die above under that mode?
Oh right. Copy/paste fail.
> > + while (getline_fn(&buf, stdin) != EOF) {
> > + if (!nul_term_line && buf.buf[0] == '"') {
> > + strbuf_reset(&unquoted);
> > + if (unquote_c_style(&unquoted, buf.buf, NULL))
> > + die(_("line is badly quoted"));
> > + strbuf_swap(&buf, &unquoted);
> > + }
> > + ALLOC_GROW(stdin_paths, stdin_nr + 1, stdin_alloc);
> > + stdin_paths[stdin_nr++] = xstrdup(buf.buf);
> > + strbuf_reset(&buf);
> > + }
> > + strbuf_release(&unquoted);
> > + strbuf_release(&buf);
> > +
> > + ALLOC_GROW(stdin_paths, stdin_nr + 1, stdin_alloc);
> > + stdin_paths[stdin_nr++] = NULL;
>
> It makes sense to collect, but...
It does, doesn't it? I really would have loved to start resetting right
away, but if the list were not sorted and traversed at the same time as
the tree-ish, the performance would just be suboptimal.
I think that is an important point and I adjusted the commit message
accordingly.
> > + parse_pathspec(&pathspec, 0, flags, prefix,
> > + (const char **)stdin_paths);
>
> ...letting them be used as if they are pathspec is wrong when
> stdin_paths[] contain wildcard, isn't it?
>
> I think flags |= PATHSPEC_LITERAL_PATH can help fixing it. 0/2 said
> this mimicks checkout-index and I think it should by not treating
> the input as wildcarded patterns (i.e. "echo '*.c' | reset --stdin"
> shouldn't be the way to reset all .c files --- that's something we
> would want to add to the test, I guess).
True. I adjust the flags accordingly now.
Thanks,
Dscho
^ 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