* RE: Switching branches not working in a cloned repo
From: Paul Williamson @ 2016-09-19 20:13 UTC (permalink / raw)
To: Jacob Keller; +Cc: git@vger.kernel.org
In-Reply-To: <CA+P7+xqR6i6eieVrEHXMNuVo6yfmRRZqX8RdVAE7y4=xSLLYyA@mail.gmail.com>
-----Original Message-----
From: Jacob Keller [mailto:jacob.keller@gmail.com]
>Try a fresh clone with "git checkout --track origin/web_dev" and then a "git status -v" and let us know what happens. I suspect that older clones have the correct branches already setup for tracking, but the new clone >isn't doing it automatically, so when you run git checkout web_dev you're just creating a new local copy.
Hi Jake, this worked!
Your explanation seems correct - I could still list the remote branches, thanks to the command that Philip suggested, but the linkage between the two was not there by default for some reason. Very strange, I have never seen that happen before.
Anyway, this solution is sound and seems to set everything (history etc) back to how it should be.
Thanks very much for your help!
Paul
^ permalink raw reply
* Re: clarification of `rev-list --no-walk ^<rev>`?
From: Philip Oakley @ 2016-09-19 20:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael J Gruber, Git List
In-Reply-To: <xmqqintrek64.fsf@gitster.mtv.corp.google.com>
From: "Junio C Hamano" <gitster@pobox.com>
> "Philip Oakley" <philipoakley@iee.org> writes:
>
>>> Philip probably has a
>>> confused notion
Hi Junio,
Could you clarify a particular point from here..
> The "--no-walk" tells the rev-list machinery "I have only positives;
> do not traverse from them AT ALL but just use these positives".
> Only under that condition, the order of the positive ends you list
> on the command line matters.
What does "--do-walk" do ("Overrides a previous --no-walk"), and when would
it be applied?
--
Philip
^ permalink raw reply
* Re: [PATCH 3/2] batch check whether submodule needs pushing into one call
From: Heiko Voigt @ 2016-09-19 20:08 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Stefan Beller, git@vger.kernel.org, Jens Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <xmqq1t0jlnm2.fsf@gitster.mtv.corp.google.com>
On Fri, Sep 16, 2016 at 11:13:09AM -0700, Junio C Hamano wrote:
> Heiko Voigt <hvoigt@hvoigt.net> writes:
> > The most exact solution would be to use all actual remote refs available
> > (not sure if we have them at this point in the process?) another
> > solution would be to still append the --remotes=<remotename> option as a
> > fallback to reduce the revisions.
>
> I'd say --remotes=<remotename> is the least problematic thing to do.
Ok then lets drop my last patch and keep it the way it was. Because if
the remote sha1 differs we probably do not have it locally anyway. The
only case this does not catch is when the user specifies a remote URL.
But that just means we will iterate over all revisions instead of a
reduced set, which makes the check slower but still correct. As one can
see from my measurements that should not be that bad anymore.
Cheers Heiko
^ permalink raw reply
* Re: [PATCH 2/2] serialize collection of refs that contain submodule changes
From: Junio C Hamano @ 2016-09-19 20:09 UTC (permalink / raw)
To: Heiko Voigt
Cc: Jeff King, Stefan Beller, git@vger.kernel.org, Jens Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <20160919195118.GB62429@book.hvoigt.net>
Heiko Voigt <hvoigt@hvoigt.net> writes:
> I am not sure if I understand you correctly here. With the "ref cache layer"
> you are referring to add_submodule_odb() which is called indirectly from
> submodule_needs_pushing()? Those revs are only used to check whether the hash
> we need on the remote side exists in the local submodule. That should not
> change due to a push. The actual check whether the commit(s) exist on the
> remote side is done using a 'rev-list' in a subprocess later.
I was wondering what would happen in this scenario:
* You have ON_DEMAND set, which causes "git -C sub push origin" to
push out what are new, updating the remote tracking branches in
the submodule, sub/.git/refs/remotes/origin/*.
* Then you check again. If you used for-each-ref-in-submodule, the
updated refs/remotes/origin/* may not have been re-read.
But you check by spawning "rev-list ... --not --remotes" as a
separate process in submodule_needs_pushing(), and that will force
the new process to read the updated state, so it turns out that I
was overly worried without good reason ;-)
It may matter once somebody tries to internalize the external
rev-list call done via start_command() interface to an internal
call, though. But we are not there yet.
^ permalink raw reply
* Re: clarification of `rev-list --no-walk ^<rev>`?
From: Junio C Hamano @ 2016-09-19 19:58 UTC (permalink / raw)
To: Philip Oakley; +Cc: Michael J Gruber, Git List
In-Reply-To: <989F47918A374EEF8C7FECD3CFC6767E@PhilipOakley>
"Philip Oakley" <philipoakley@iee.org> writes:
>> Philip probably has a
>> confused notion that "rev-list A..B C..D" is somehow a union of set
>> A..B and C..D?
>
> That wasn't the issue. Though it does beg the question that it's the
> same as "rev-list D B ^A ^C" isn't it?
If you think it begs the question, then you haven't understood what
I meant by all of the explanation. Let me repeat:
"A..B C..D" is exactly a short-hand for "^A B ^C D" which is
the same as ANY permutation like "D B ^A ^C".
This is because rev-list machinery NEVER does "ah the command line
asks for two or more sets to be computed and then a union of the
results to be returned". As I said, it collects positive starting
points (B and D) and negative starting points (A and C), and does a
single traversal "ones that are reachable from positives, excluding
the ones that are reachable from negatives". That is all it does.
Once that is understood, it should be obvious that it would not have
any effect on the result to permute the order of positives and
negatives.
> I'd say it was the walk - no walk range confusion.
You may _additionally_ have had that confusion ;-).
> i.e. cherry-pick B D F G Q..T; isn't B D F G R S T, is it?
Of course it isn't, but that is not about any ordering. It is
because the machinery does not work this way:
- we have positives B D F G and ... oh wait, we see Q..T, so we
need to traverse ONLY that bit and come up with positives in the
resulting set, which are R S T, and take a union of all of the
above; and then
- now we process --no-walk to show only the resulting positives.
The "--no-walk" tells the rev-list machinery "I have only positives;
do not traverse from them AT ALL but just use these positives".
Only under that condition, the order of the positive ends you list
on the command line matters.
Having ^Q in the input "B D F G ^Q T" contradicts that "I have only
positives" precondtion and forces the machinery to traverse, i.e. to
find things reachable from B D F G T excluding things reachable from
Q. The order of the output would be topological (i.e. a parent is
not shown until all its children are), with or without --date-order,
and may not match the order you gave the positive ones from the
command line.
You may want the "exclude things reachable from Q" part to not have
any effect on things related to B, D, F or G, but the machinery does
not work that way. That is because there is only one single traversal
and no union operation involved.
It is a different matter Git born in a fictional world that takes
A..B C..D as a union of two sets is better than the current one.
There might be cases where such a variant of "a set of commits that
is a union of multiple sets" is handy to use. But I think we are
talking about what Git in our world does, so I'd not go there in
this discussion.
^ permalink raw reply
* Re: [PATCH 3/2] batch check whether submodule needs pushing into one call
From: Heiko Voigt @ 2016-09-19 19:58 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Stefan Beller, git@vger.kernel.org, Jens Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <xmqq8turlo8m.fsf@gitster.mtv.corp.google.com>
On Fri, Sep 16, 2016 at 10:59:37AM -0700, Junio C Hamano wrote:
> Heiko Voigt <hvoigt@hvoigt.net> writes:
> > +static void check_has_hash(const unsigned char sha1[20], void *data)
> > +{
> > + int *has_hash = (int *) data;
> > +
> > + if (!lookup_commit_reference(sha1))
> > + *has_hash = 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;
>
> I think you meant well, but this optimization is wrong. A mere
> presence of an object does not mean that the current tip can reach
> that object. Imagine you pushed commit A earlier to them at the
> tip, then pushed commit A~ to them at the tip, which is the current
> state of the remote of the submodule, and since them they may have
> GC'ed. They no longer have the commit A.
>
> For that matter, because you are doing this check by pretending as
> if all the submodule objects are in the object store of the current
> superproject you are working in, and saying "it exists there in the
> submodule repository" when the only thing you know is it exists in
> an object store of either the submodule repository, the superproject
> repository, or any of the other submodule repositories, you really
> cannot tell much from a mere presence of an object. Not just the
> remote of the submodule repository you are interested in, but the
> submodule repository you are interested in itself, may not have that
> object.
>
> Drop the previous two helper functions and this short-cut.
This is nothing I added. This came from the original code which I simply
extended to check for all sha1's. The diff is a little bit misleading. This
would be the local diff:
- if (add_submodule_odb(path) || !lookup_commit_reference(sha1))
+ if (!submodule_has_hashes(path, hashes))
return 0;
I think that this is more a shortcut for: If we can not find the sha1, we do
not need to bother spawning a process for the real check. We default to the
answer no in that case.
It looks like the reason for the sha1 check is to avoid error output from the
spawned 'rev-list' process. The error output might be confusing to the user in
case the sha1 does not exist in the submodule. We should probably die here
since we were not able to check a submodule that has a diff in the revisions we
would push.
After thinking about this further: I think it is actually bad to proceed here,
as the current revisions (just in the superproject) would still be pushed and
the user possibly gets an inconsistent state on the remote side which he tried
to avoid with check/on-demand-push enabled.
So in short this deserves some further love. Will have a look into adding
another patch for this if we agree on my suggestion to die above.
Cheers Heiko
^ permalink raw reply
* Re: [PATCH 2/2] serialize collection of refs that contain submodule changes
From: Heiko Voigt @ 2016-09-19 19:51 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Stefan Beller, git@vger.kernel.org, Jens Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <xmqqd1k3losd.fsf@gitster.mtv.corp.google.com>
Hi,
On Fri, Sep 16, 2016 at 10:47:46AM -0700, Junio C Hamano wrote:
> One thing that makes me worried is how the ref cache layer interacts
> with this. I see you first call push_unpushed_submodules() when
> ON_DEMAND is set, which would result in pushes in submodule
> repositories, updating their remote tracking branches. At that
> point, before you make another call to find_unpushed_submodules(),
> is our cached ref layer knows that the remote tracking branches
> are now up to date (otherwise, we would incorrectly judge that these
> submodules need pushing based on stale information)?
I am not sure if I understand you correctly here. With the "ref cache layer"
you are referring to add_submodule_odb() which is called indirectly from
submodule_needs_pushing()? Those revs are only used to check whether the hash
we need on the remote side exists in the local submodule. That should not
change due to a push. The actual check whether the commit(s) exist on the
remote side is done using a 'rev-list' in a subprocess later.
> > diff --git a/transport.c b/transport.c
> > index 94d6dc3..76e1daf 100644
> > --- a/transport.c
> > +++ b/transport.c
> > @@ -903,23 +903,29 @@ int transport_push(struct transport *transport,
> >
> > if ((flags & TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND) && !is_bare_repository()) {
> > struct ref *ref = remote_refs;
> > + struct sha1_array hashes = SHA1_ARRAY_INIT;
> > +
> > for (; ref; ref = ref->next)
> > - if (!is_null_oid(&ref->new_oid) &&
> > - !push_unpushed_submodules(ref->new_oid.hash,
> > - transport->remote->name))
> > - die ("Failed to push all needed submodules!");
> > + if (!is_null_oid(&ref->new_oid))
> > + sha1_array_append(&hashes, ref->new_oid.hash);
> > +
> > + if (!push_unpushed_submodules(&hashes, transport->remote->name))
> > + die ("Failed to push all needed submodules!");
>
> Do we leak the contents of hashes here?
Good catch, sorry about that. Will clear the hashes array.
> > }
> >
> > if ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
> > TRANSPORT_RECURSE_SUBMODULES_CHECK)) && !is_bare_repository()) {
> > struct ref *ref = remote_refs;
> > struct string_list needs_pushing = STRING_LIST_INIT_DUP;
> > + struct sha1_array hashes = SHA1_ARRAY_INIT;
> >
> > for (; ref; ref = ref->next)
> > - if (!is_null_oid(&ref->new_oid) &&
> > - find_unpushed_submodules(ref->new_oid.hash,
> > - transport->remote->name, &needs_pushing))
> > - die_with_unpushed_submodules(&needs_pushing);
> > + if (!is_null_oid(&ref->new_oid))
> > + sha1_array_append(&hashes, ref->new_oid.hash);
> > +
> > + if (find_unpushed_submodules(&hashes, transport->remote->name,
> > + &needs_pushing))
> > + die_with_unpushed_submodules(&needs_pushing);
>
> Do we leak the contents of hashes here? I do not think we need to
> worry about needs_pushing leaking, as we will always die if it is
> not empty, but it might be a better code hygiene to clear it as
> well.
As above, will fix.
Cheers Heiko
^ permalink raw reply
* Re: Switching branches not working in a cloned repo
From: Jacob Keller @ 2016-09-19 19:50 UTC (permalink / raw)
To: Paul Williamson; +Cc: git@vger.kernel.org
In-Reply-To: <YQXPR01MB0023CDCFA27BDD4959B4F9B19EF40@YQXPR01MB0023.CANPRD01.PROD.OUTLOOK.COM>
On Mon, Sep 19, 2016 at 6:58 AM, Paul Williamson
<paul.williamson@mediamiser.com> wrote:
> Hi,
>
> We use git extensively on a number of repos. Recently, we have had a problem with one of them. This repo has a 'web_dev' branch. For copies of the repo cloned before a certain (recent but unidentified) time, we could 'git checkout' between master and web_dev and everything would be normal.
>
> However, now if we clone the repo, we can no longer do 'git checkout web_dev'. Git doesn't complain, in fact there is no feedback and we are still in the master branch. Running 'git branch -r' still shows the branch as existing at origin.
>
> If we try 'git branch web_dev' we then see web_dev listed locally and can switch to it BUT on closer inspection we realise that this action has created a new branch off master.
>
> The first time we saw this was under Bash on Windows, so we thought maybe it was a beta problem, but a) other repos work as expected under that environment, and b) under cygwin, pulling the same repo to a new directory alongside an older copy shows that the problem occurs with the new clone, but not the one that that was cloned longer ago.
>
> Also in this situation, there are no local outstanding code changes that might cause problems switching branches. This occurs right from a cleanly cloned repo.
>
> It seems something has gone wrong with this repo, and we don't know what. It's a tough problem to google, and I was not able to search the gmane archives (DNS errors).
>
> Any idea how to investigate?
>
Try a fresh clone with "git checkout --track origin/web_dev" and then
a "git status -v" and let us know what happens. I suspect that older
clones have the correct branches already setup for tracking, but the
new clone isn't doing it automatically, so when you run git checkout
web_dev you're just creating a new local copy.
You can probably fix this by running
git checkout web_dev
git branch --set-upstream-to=origin/web_dev
git reset --hard origin/web_dev
The last command will completely reset your local web_dev to match
what's upstream, and the second command tells git that this branch
tracks a given remote branch.
Hope this helps.
Regards,
Jake
> Thanks,
> Paul
>
>
^ permalink raw reply
* Re: [PATCH 1/2] serialize collection of changed submodules
From: Heiko Voigt @ 2016-09-19 19:44 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Stefan Beller, git@vger.kernel.org, Jens Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <xmqqintvlpqv.fsf@gitster.mtv.corp.google.com>
On Fri, Sep 16, 2016 at 10:27:04AM -0700, Junio C Hamano wrote:
> Heiko Voigt <hvoigt@hvoigt.net> writes:
>
> > +static struct sha1_array *get_sha1s_from_list(struct string_list *submodules,
> > + const char *path)
> > +{
> > + struct string_list_item *item;
> > + struct sha1_array *hashes;
> > +
> > + item = string_list_insert(submodules, path);
> > + if (item->util)
> > + return (struct sha1_array *) item->util;
> > +
> > + hashes = (struct sha1_array *) xmalloc(sizeof(struct sha1_array));
> > + /* NEEDSWORK: should we add an initializer function for
> > + * sha1_array ? */
> > + memset(hashes, 0, sizeof(struct sha1_array));
> > + item->util = hashes;
>
>
> /* NEEDSWORK: should we have SHA1_ARRAY_INIT etc.? */
> item->util = xcalloc(1, sizeof(struct sha1_array));
Ok will do.
^ permalink raw reply
* Re: clarification of `rev-list --no-walk ^<rev>`?
From: Philip Oakley @ 2016-09-19 19:31 UTC (permalink / raw)
To: Junio C Hamano, Michael J Gruber; +Cc: Git List
In-Reply-To: <xmqq8tunhns4.fsf@gitster.mtv.corp.google.com>
From: "Junio C Hamano" <gitster@pobox.com>
> Michael J Gruber <git@drmicha.warpmail.net> writes:
>
>>> It can be read that
>>>
>>> $ git cherry-pick maint next
>>>
>>> would pick two single commits, while
>>>
>>> $ git cherry-pick maint next ^master
>>>
>>> could implicitly be read as
>>>
>>> $ git cherry-pick maint next --do-walk ^master
>
> You can read it as "master..next maint" that does force walking.
>
>>> Clearly that's not what is intended, which is
>>>
>>> $ git cherry-pick --do-walk maint next ^master
>
> I do not see the distinction betwee the above two you seem to be
> trying to make. Care to explain?
>
>>> but it is open to interpretation as to where in the command line the
>>> caret
>>> range prefix's --do-walk (to countermand the --no-walk) should applied.
>
> I do not think it can be position dependent.
OK. (background, long story) When I first read the man page, and in trying
to explain a user confusion between cherry pick commits and am'ing the same
commits via format-patch (where sometimes the patches had coverlapping
context issues), I was trying to confirm for myself that 'git cherry-pick B
C' and 'git cherry-pick C B' should get the same end result, and not be
mistaken (e.g. user misunderstanding) for a range.
I first spotted the 'git help cherry-pick's line:
<commit> - no traversal is done by default, as if the --no-walk option was
specified, see git-rev-list(1).
So off I goes to rev-list, and see that the options no-walk/do-walk are said
(implied) to be position dependent.
--do-walk - Overrides a previous --no-walk.
Meanwhile the --no-walk option says:
--no-walk - This (option) has no effect if a range is specified.
So at this point I am wondering about the command line ordering and what
comprises the range if the negative ref is given last, or at least just
before a --do-walk.
Thus (at this point) it felt like one of those specification rabbit-holes
that I often see at $dayjob. It was unclear as to the point at which the
'range' was to be applied in to the command line to get the expected
examples.
Climbing back out of the rabbit-hole. I now see that after the end of the
cherry-pick <commit> line I quoted, there is a secondary "Note that
specifying a range will feed all <commit>. arguments to a single revision
walk", which at the time did not register (a classic human error.. Three
Mile Island et al.).
Similarly, in the rev-list --no-walk option, its says (mid-para) "This has
no effect if a range is specified." so in some ways that confirms the cherry
pick statement, but again a less obvious corollary.
However, there is still a small step missing, which is to confirm that using
a negative ^ref anywhere(?) makes the whole list of refs into a range (i.e.
it will look-back along the command line) to cancel any --no-walk options in
place.
Given that a walk usually requires a range, I'm now having difficulty seeing
how the --no-walk <revs> --do-walk can be combined anyway.
The bits I felt was missing (in the docs) was to say explicitly somewhere
that a negative ref defined that we had a range (to link back to those
walk-no-walk statements), and the extent of rev paramaters it applied to.
And after re-reading, that some of those corollary statements about ranges
flipping the walk-no-walk condition should be brought forward to be more
obvious within the primary rev-list (to avoid the typical reader error).
> Philip probably has a
> confused notion that "rev-list A..B C..D" is somehow a union of set
> A..B and C..D?
That wasn't the issue. Though it does beg the question that it's the same as
"rev-list D B ^A ^C" isn't it?
>
>>> If the user did want just the single commit at the tip of maint, and
>>> then
>>> the range master..next, what would be their command line, and also, how
>>> would the man page warn against false expectations?
>
> Yeah, this can show us that all of the have is coming from that
> exact confusion I suspected Philip has. We need to clarify in the
> documentation that rev-list set operation does *NOT* have union of
> multiple sets to unconfuse the readers.
I'd say it was the walk - no walk range confusion. Inclusion of any range
definition of any sort (in particular ^rev) causes the expectation that an
ordered list of single revs can be included, to be broken.
i.e. cherry-pick B D F G Q..T; isn't B D F G R S T, is it?
I've also have a quick browse of the test scripts and didn't see any tests
that actually cover the example of `git cherry-pick maint next ^master`
where both have multiple commits to pick, so couldn't see what the test
would expect.
--
Philip
^ permalink raw reply
* [PATCH v2 1/2] t5100-mailinfo: replace common path prefix with variable
From: Kevin Daudt @ 2016-09-19 18:54 UTC (permalink / raw)
To: git; +Cc: Kevin Daudt, Swift Geek, Jeff King, Junio C Hamano
In-Reply-To: <20160916210204.31282-1-me@ikke.info>
Many tests need to store data in a file, and repeat the same pattern to
refer to that path:
"$TEST_DATA"/t5100/
Create a variable that contains this path, and use that instead.
Signed-off-by: Kevin Daudt <me@ikke.info>
---
t/t5100-mailinfo.sh | 56 +++++++++++++++++++++++++++--------------------------
1 file changed, 29 insertions(+), 27 deletions(-)
diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index 1a5a546..27bf3b8 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -7,8 +7,10 @@ test_description='git mailinfo and git mailsplit test'
. ./test-lib.sh
+DATA="$TEST_DIRECTORY/t5100"
+
test_expect_success 'split sample box' \
- 'git mailsplit -o. "$TEST_DIRECTORY"/t5100/sample.mbox >last &&
+ 'git mailsplit -o. "$DATA"/sample.mbox >last &&
last=$(cat last) &&
echo total is $last &&
test $(cat last) = 17'
@@ -17,9 +19,9 @@ check_mailinfo () {
mail=$1 opt=$2
mo="$mail$opt"
git mailinfo -u $opt msg$mo patch$mo <$mail >info$mo &&
- test_cmp "$TEST_DIRECTORY"/t5100/msg$mo msg$mo &&
- test_cmp "$TEST_DIRECTORY"/t5100/patch$mo patch$mo &&
- test_cmp "$TEST_DIRECTORY"/t5100/info$mo info$mo
+ test_cmp "$DATA"/msg$mo msg$mo &&
+ test_cmp "$DATA"/patch$mo patch$mo &&
+ test_cmp "$DATA"/info$mo info$mo
}
@@ -27,15 +29,15 @@ for mail in 00*
do
test_expect_success "mailinfo $mail" '
check_mailinfo $mail "" &&
- if test -f "$TEST_DIRECTORY"/t5100/msg$mail--scissors
+ if test -f "$DATA"/msg$mail--scissors
then
check_mailinfo $mail --scissors
fi &&
- if test -f "$TEST_DIRECTORY"/t5100/msg$mail--no-inbody-headers
+ if test -f "$DATA"/msg$mail--no-inbody-headers
then
check_mailinfo $mail --no-inbody-headers
fi &&
- if test -f "$TEST_DIRECTORY"/t5100/msg$mail--message-id
+ if test -f "$DATA"/msg$mail--message-id
then
check_mailinfo $mail --message-id
fi
@@ -45,7 +47,7 @@ done
test_expect_success 'split box with rfc2047 samples' \
'mkdir rfc2047 &&
- git mailsplit -orfc2047 "$TEST_DIRECTORY"/t5100/rfc2047-samples.mbox \
+ git mailsplit -orfc2047 "$DATA"/rfc2047-samples.mbox \
>rfc2047/last &&
last=$(cat rfc2047/last) &&
echo total is $last &&
@@ -56,18 +58,18 @@ do
test_expect_success "mailinfo $mail" '
git mailinfo -u $mail-msg $mail-patch <$mail >$mail-info &&
echo msg &&
- test_cmp "$TEST_DIRECTORY"/t5100/empty $mail-msg &&
+ test_cmp "$DATA"/empty $mail-msg &&
echo patch &&
- test_cmp "$TEST_DIRECTORY"/t5100/empty $mail-patch &&
+ test_cmp "$DATA"/empty $mail-patch &&
echo info &&
- test_cmp "$TEST_DIRECTORY"/t5100/rfc2047-info-$(basename $mail) $mail-info
+ test_cmp "$DATA"/rfc2047-info-$(basename $mail) $mail-info
'
done
test_expect_success 'respect NULs' '
- git mailsplit -d3 -o. "$TEST_DIRECTORY"/t5100/nul-plain &&
- test_cmp "$TEST_DIRECTORY"/t5100/nul-plain 001 &&
+ git mailsplit -d3 -o. "$DATA"/nul-plain &&
+ test_cmp "$DATA"/nul-plain 001 &&
(cat 001 | git mailinfo msg patch) &&
test_line_count = 4 patch
@@ -75,52 +77,52 @@ test_expect_success 'respect NULs' '
test_expect_success 'Preserve NULs out of MIME encoded message' '
- git mailsplit -d5 -o. "$TEST_DIRECTORY"/t5100/nul-b64.in &&
- test_cmp "$TEST_DIRECTORY"/t5100/nul-b64.in 00001 &&
+ git mailsplit -d5 -o. "$DATA"/nul-b64.in &&
+ test_cmp "$DATA"/nul-b64.in 00001 &&
git mailinfo msg patch <00001 &&
- test_cmp "$TEST_DIRECTORY"/t5100/nul-b64.expect patch
+ test_cmp "$DATA"/nul-b64.expect patch
'
test_expect_success 'mailinfo on from header without name works' '
mkdir info-from &&
- git mailsplit -oinfo-from "$TEST_DIRECTORY"/t5100/info-from.in &&
- test_cmp "$TEST_DIRECTORY"/t5100/info-from.in info-from/0001 &&
+ git mailsplit -oinfo-from "$DATA"/info-from.in &&
+ test_cmp "$DATA"/info-from.in info-from/0001 &&
git mailinfo info-from/msg info-from/patch \
<info-from/0001 >info-from/out &&
- test_cmp "$TEST_DIRECTORY"/t5100/info-from.expect info-from/out
+ test_cmp "$DATA"/info-from.expect info-from/out
'
test_expect_success 'mailinfo finds headers after embedded From line' '
mkdir embed-from &&
- git mailsplit -oembed-from "$TEST_DIRECTORY"/t5100/embed-from.in &&
- test_cmp "$TEST_DIRECTORY"/t5100/embed-from.in embed-from/0001 &&
+ git mailsplit -oembed-from "$DATA"/embed-from.in &&
+ test_cmp "$DATA"/embed-from.in embed-from/0001 &&
git mailinfo embed-from/msg embed-from/patch \
<embed-from/0001 >embed-from/out &&
- test_cmp "$TEST_DIRECTORY"/t5100/embed-from.expect embed-from/out
+ test_cmp "$DATA"/embed-from.expect embed-from/out
'
test_expect_success 'mailinfo on message with quoted >From' '
mkdir quoted-from &&
- git mailsplit -oquoted-from "$TEST_DIRECTORY"/t5100/quoted-from.in &&
- test_cmp "$TEST_DIRECTORY"/t5100/quoted-from.in quoted-from/0001 &&
+ git mailsplit -oquoted-from "$DATA"/quoted-from.in &&
+ test_cmp "$DATA"/quoted-from.in quoted-from/0001 &&
git mailinfo quoted-from/msg quoted-from/patch \
<quoted-from/0001 >quoted-from/out &&
- test_cmp "$TEST_DIRECTORY"/t5100/quoted-from.expect quoted-from/msg
+ test_cmp "$DATA"/quoted-from.expect quoted-from/msg
'
test_expect_success 'mailinfo unescapes with --mboxrd' '
mkdir mboxrd &&
git mailsplit -omboxrd --mboxrd \
- "$TEST_DIRECTORY"/t5100/sample.mboxrd >last &&
+ "$DATA"/sample.mboxrd >last &&
test x"$(cat last)" = x2 &&
for i in 0001 0002
do
git mailinfo mboxrd/msg mboxrd/patch \
<mboxrd/$i >mboxrd/out &&
- test_cmp "$TEST_DIRECTORY"/t5100/${i}mboxrd mboxrd/msg
+ test_cmp "$DATA"/${i}mboxrd mboxrd/msg
done &&
sp=" " &&
echo "From " >expect &&
--
2.10.0.86.g6ffa4f1.dirty
^ permalink raw reply related
* [PATCH v2 2/2] mailinfo: unescape quoted-pair in header fields
From: Kevin Daudt @ 2016-09-19 18:54 UTC (permalink / raw)
To: git; +Cc: Kevin Daudt, Swift Geek, Jeff King, Junio C Hamano
In-Reply-To: <20160916210204.31282-1-me@ikke.info>
rfc2822 has provisions for quoted strings in structured header fields,
but also allows for escaping these with so-called quoted-pairs.
The only thing git currently does is removing exterior quotes, but
quotes within are left alone.
Remove exterior quotes and remove escape characters so that they don't
show up in the author field.
Signed-off-by: Kevin Daudt <me@ikke.info>
---
mailinfo.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
t/t5100-mailinfo.sh | 14 ++++++++++++++
| 5 +++++
| 9 +++++++++
t/t5100/quoted-string.expect | 5 +++++
t/t5100/quoted-string.in | 9 +++++++++
6 files changed, 88 insertions(+)
create mode 100644 t/t5100/comment.expect
create mode 100644 t/t5100/comment.in
create mode 100644 t/t5100/quoted-string.expect
create mode 100644 t/t5100/quoted-string.in
diff --git a/mailinfo.c b/mailinfo.c
index e19abe3..6a7c2f2 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -54,6 +54,50 @@ static void parse_bogus_from(struct mailinfo *mi, const struct strbuf *line)
get_sane_name(&mi->name, &mi->name, &mi->email);
}
+static void unquote_quoted_string(struct strbuf *line)
+{
+ const char *in = strbuf_detach(line, NULL);
+ int c, take_next_literally = 0;
+ int found_error = 0;
+
+ /*
+ * Stores the character that started the escape mode so that we know what
+ * character will stop it
+ */
+ char escape_context = 0;
+
+ while ((c = *in++) != 0) {
+ if (take_next_literally) {
+ take_next_literally = 0;
+ } else {
+ switch (c) {
+ case '"':
+ if (!escape_context)
+ escape_context = '"';
+ else if (escape_context == '"')
+ escape_context = 0;
+ continue;
+ case '\\':
+ if (escape_context) {
+ take_next_literally = 1;
+ continue;
+ }
+ break;
+ case '(':
+ if (!escape_context)
+ escape_context = '(';
+ break;
+ case ')':
+ if (escape_context == '(')
+ escape_context = 0;
+ break;
+ }
+ }
+
+ strbuf_addch(line, c);
+ }
+}
+
static void handle_from(struct mailinfo *mi, const struct strbuf *from)
{
char *at;
@@ -63,6 +107,8 @@ static void handle_from(struct mailinfo *mi, const struct strbuf *from)
strbuf_init(&f, from->len);
strbuf_addbuf(&f, from);
+ unquote_quoted_string(&f);
+
at = strchr(f.buf, '@');
if (!at) {
parse_bogus_from(mi, from);
diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index 27bf3b8..8c21434 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -144,4 +144,18 @@ test_expect_success 'mailinfo unescapes with --mboxrd' '
test_cmp expect mboxrd/msg
'
+test_expect_success 'mailinfo handles rfc2822 quoted-string' '
+ mkdir quoted-string &&
+ git mailinfo /dev/null /dev/null <"$DATA"/quoted-string.in \
+ >quoted-string/info &&
+ test_cmp "$DATA"/quoted-string.expect quoted-string/info
+'
+
+test_expect_success 'mailinfo handles rfc2822 comment' '
+ mkdir comment &&
+ git mailinfo /dev/null /dev/null <"$DATA"/comment.in \
+ >comment/info &&
+ test_cmp "$DATA"/comment.expect comment/info
+'
+
test_done
--git a/t/t5100/comment.expect b/t/t5100/comment.expect
new file mode 100644
index 0000000..1197e76
--- /dev/null
+++ b/t/t5100/comment.expect
@@ -0,0 +1,5 @@
+Author: A U Thor (this is a comment (really))
+Email: somebody@example.com
+Subject: testing comments
+Date: Sun, 25 May 2008 00:38:18 -0700
+
--git a/t/t5100/comment.in b/t/t5100/comment.in
new file mode 100644
index 0000000..430ba97
--- /dev/null
+++ b/t/t5100/comment.in
@@ -0,0 +1,9 @@
+From 1234567890123456789012345678901234567890 Mon Sep 17 00:00:00 2001
+From: "A U Thor" <somebody@example.com> (this is a comment \(really\))
+Date: Sun, 25 May 2008 00:38:18 -0700
+Subject: [PATCH] testing comments
+
+
+
+---
+patch
diff --git a/t/t5100/quoted-string.expect b/t/t5100/quoted-string.expect
new file mode 100644
index 0000000..cab1bce
--- /dev/null
+++ b/t/t5100/quoted-string.expect
@@ -0,0 +1,5 @@
+Author: Author "The Author" Name
+Email: somebody@example.com
+Subject: testing quoted-pair
+Date: Sun, 25 May 2008 00:38:18 -0700
+
diff --git a/t/t5100/quoted-string.in b/t/t5100/quoted-string.in
new file mode 100644
index 0000000..e2e627a
--- /dev/null
+++ b/t/t5100/quoted-string.in
@@ -0,0 +1,9 @@
+From 1234567890123456789012345678901234567890 Mon Sep 17 00:00:00 2001
+From: "Author \"The Author\" Name" <somebody@example.com>
+Date: Sun, 25 May 2008 00:38:18 -0700
+Subject: [PATCH] testing quoted-pair
+
+
+
+---
+patch
--
2.10.0.86.g6ffa4f1.dirty
^ permalink raw reply related
* [PATCH v2 0/2] Handle escape characters in From field.
From: Kevin Daudt @ 2016-09-19 18:54 UTC (permalink / raw)
To: git; +Cc: Kevin Daudt, Swift Geek, Jeff King, Junio C Hamano
In-Reply-To: <20160916210204.31282-1-me@ikke.info>
Changes since v2:
- detach from input parameter to reuse it as an output buffer
- don't return error when encountering another open bracket in a comment
- test escaping in comments
Kevin Daudt (2):
t5100-mailinfo: replace common path prefix with variable
mailinfo: unescape quoted-pair in header fields
mailinfo.c | 46 +++++++++++++++++++++++++++++
t/t5100-mailinfo.sh | 70 +++++++++++++++++++++++++++-----------------
t/t5100/comment.expect | 5 ++++
t/t5100/comment.in | 9 ++++++
t/t5100/quoted-string.expect | 5 ++++
t/t5100/quoted-string.in | 9 ++++++
6 files changed, 117 insertions(+), 27 deletions(-)
create mode 100644 t/t5100/comment.expect
create mode 100644 t/t5100/comment.in
create mode 100644 t/t5100/quoted-string.expect
create mode 100644 t/t5100/quoted-string.in
--
2.10.0.86.g6ffa4f1.dirty
^ permalink raw reply
* [PATCH v2] ls-files: add pathspec matching for submodules
From: Brandon Williams @ 2016-09-19 18:52 UTC (permalink / raw)
To: git; +Cc: Brandon Williams
In-Reply-To: <CAKoko1qrAuLhn6qQax-BSZFhEqbFdiBbVrip8TD3NVjD8Xzy0g@mail.gmail.com>
Pathspecs can be a bit tricky when trying to apply them to submodules.
The main challenge is that the pathspecs will be with respect to the
super module and not with respect to paths in the submodule. The
approach this patch takes is to pass in the identical pathspec from the
super module to the submodule in addition to the submodule-prefix, which
is the path from the root of the super module to the submodule, and then
we can compare an entry in the submodule prepended with the
submodule-prefix to the pathspec in order to determine if there is a
match.
This patch also permits the pathspec logic to perform a prefix match against
submodules since a pathspec could refer to a file inside of a submodule.
This can can result in false positive matches since a super module
doesn't know what files could be in the submodule.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
Documentation/git-ls-files.txt | 4 +-
builtin/ls-files.c | 144 ++++++++++++++++++++-------------
dir.c | 67 ++++++++++++++-
dir.h | 4 +
t/t3007-ls-files-recurse-submodules.sh | 66 +++++++++++++--
5 files changed, 215 insertions(+), 70 deletions(-)
diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index a623ebf..09e4449 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -19,7 +19,7 @@ SYNOPSIS
[--exclude-standard]
[--error-unmatch] [--with-tree=<tree-ish>]
[--full-name] [--recurse-submodules]
- [--output-path-prefix=<path>]
+ [--submodule-prefix=<path>]
[--abbrev] [--] [<file>...]
DESCRIPTION
@@ -143,7 +143,7 @@ a space) at the start of each line:
Recursively calls ls-files on each submodule in the repository.
Currently there is only support for the --cached mode.
---output-path-prefix=<path>::
+--submodule-prefix=<path>::
Prepend the provided path to the output of each file
--abbrev[=<n>]::
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 687e475..1417f44 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -29,7 +29,7 @@ static int show_valid_bit;
static int line_terminator = '\n';
static int debug_mode;
static int show_eol;
-static const char *output_path_prefix;
+static const char *submodule_prefix;
static int recurse_submodules;
static const char *prefix;
@@ -78,9 +78,9 @@ static void write_name(const char *name)
* churn.
*/
static struct strbuf full_name = STRBUF_INIT;
- if (output_path_prefix && *output_path_prefix) {
+ if (submodule_prefix && *submodule_prefix) {
strbuf_reset(&full_name);
- strbuf_addstr(&full_name, output_path_prefix);
+ strbuf_addstr(&full_name, submodule_prefix);
strbuf_addstr(&full_name, name);
name = full_name.buf;
}
@@ -177,12 +177,34 @@ static void show_gitlink(const struct cache_entry *ce)
{
struct child_process cp = CHILD_PROCESS_INIT;
int status;
+ int i;
argv_array_push(&cp.args, "ls-files");
argv_array_push(&cp.args, "--recurse-submodules");
- argv_array_pushf(&cp.args, "--output-path-prefix=%s%s/",
- output_path_prefix ? output_path_prefix : "",
+ argv_array_pushf(&cp.args, "--submodule-prefix=%s%s/",
+ submodule_prefix ? submodule_prefix : "",
ce->name);
+ /* add options */
+ if (show_eol)
+ argv_array_push(&cp.args, "--eol");
+ if (show_valid_bit)
+ argv_array_push(&cp.args, "-v");
+ if (show_stage)
+ argv_array_push(&cp.args, "--stage");
+ if (show_cached)
+ argv_array_push(&cp.args, "--cached");
+ if (debug_mode)
+ argv_array_push(&cp.args, "--debug");
+
+ /*
+ * Pass in the original pathspec args. The submodule will be
+ * responsible for prepending the 'submodule_prefix' prior to comparing
+ * against the pathspec for matches.
+ */
+ argv_array_push(&cp.args, "--");
+ for (i = 0; i < pathspec.nr; ++i)
+ argv_array_push(&cp.args, pathspec.items[i].original);
+
cp.git_cmd = 1;
cp.dir = ce->name;
status = run_command(&cp);
@@ -192,57 +214,62 @@ static void show_gitlink(const struct cache_entry *ce)
static void show_ce_entry(const char *tag, const struct cache_entry *ce)
{
+ struct strbuf name = STRBUF_INIT;
int len = max_prefix_len;
+ if (submodule_prefix)
+ strbuf_addstr(&name, submodule_prefix);
+ strbuf_addstr(&name, ce->name);
if (len >= ce_namelen(ce))
die("git ls-files: internal error - cache entry not superset of prefix");
- if (!match_pathspec(&pathspec, ce->name, ce_namelen(ce),
- len, ps_matched,
- S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode)))
- return;
- if (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {
+ if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&
+ submodule_path_match(&pathspec, name.buf, ps_matched)) {
show_gitlink(ce);
- return;
- }
+ } else if (match_pathspec(&pathspec, name.buf, name.len,
+ len, ps_matched,
+ S_ISDIR(ce->ce_mode) ||
+ S_ISGITLINK(ce->ce_mode))) {
+ if (tag && *tag && show_valid_bit &&
+ (ce->ce_flags & CE_VALID)) {
+ static char alttag[4];
+ memcpy(alttag, tag, 3);
+ if (isalpha(tag[0]))
+ alttag[0] = tolower(tag[0]);
+ else if (tag[0] == '?')
+ alttag[0] = '!';
+ else {
+ alttag[0] = 'v';
+ alttag[1] = tag[0];
+ alttag[2] = ' ';
+ alttag[3] = 0;
+ }
+ tag = alttag;
+ }
- if (tag && *tag && show_valid_bit &&
- (ce->ce_flags & CE_VALID)) {
- static char alttag[4];
- memcpy(alttag, tag, 3);
- if (isalpha(tag[0]))
- alttag[0] = tolower(tag[0]);
- else if (tag[0] == '?')
- alttag[0] = '!';
- else {
- alttag[0] = 'v';
- alttag[1] = tag[0];
- alttag[2] = ' ';
- alttag[3] = 0;
+ if (!show_stage) {
+ fputs(tag, stdout);
+ } else {
+ printf("%s%06o %s %d\t",
+ tag,
+ ce->ce_mode,
+ find_unique_abbrev(ce->sha1,abbrev),
+ ce_stage(ce));
+ }
+ write_eolinfo(ce, ce->name);
+ write_name(ce->name);
+ if (debug_mode) {
+ const struct stat_data *sd = &ce->ce_stat_data;
+
+ printf(" ctime: %d:%d\n", sd->sd_ctime.sec, sd->sd_ctime.nsec);
+ printf(" mtime: %d:%d\n", sd->sd_mtime.sec, sd->sd_mtime.nsec);
+ printf(" dev: %d\tino: %d\n", sd->sd_dev, sd->sd_ino);
+ printf(" uid: %d\tgid: %d\n", sd->sd_uid, sd->sd_gid);
+ printf(" size: %d\tflags: %x\n", sd->sd_size, ce->ce_flags);
}
- tag = alttag;
}
- if (!show_stage) {
- fputs(tag, stdout);
- } else {
- printf("%s%06o %s %d\t",
- tag,
- ce->ce_mode,
- find_unique_abbrev(ce->sha1,abbrev),
- ce_stage(ce));
- }
- write_eolinfo(ce, ce->name);
- write_name(ce->name);
- if (debug_mode) {
- const struct stat_data *sd = &ce->ce_stat_data;
-
- printf(" ctime: %d:%d\n", sd->sd_ctime.sec, sd->sd_ctime.nsec);
- printf(" mtime: %d:%d\n", sd->sd_mtime.sec, sd->sd_mtime.nsec);
- printf(" dev: %d\tino: %d\n", sd->sd_dev, sd->sd_ino);
- printf(" uid: %d\tgid: %d\n", sd->sd_uid, sd->sd_gid);
- printf(" size: %d\tflags: %x\n", sd->sd_size, ce->ce_flags);
- }
+ strbuf_release(&name);
}
static void show_ru_info(void)
@@ -510,7 +537,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
{ OPTION_SET_INT, 0, "full-name", &prefix_len, NULL,
N_("make the output relative to the project top directory"),
PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL },
- OPT_STRING(0, "output-path-prefix", &output_path_prefix,
+ OPT_STRING(0, "submodule-prefix", &submodule_prefix,
N_("path"), N_("prepend <path> to each file")),
OPT_BOOL(0, "recurse-submodules", &recurse_submodules,
N_("recurse through submodules")),
@@ -566,27 +593,28 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
setup_work_tree();
if (recurse_submodules &&
- (show_stage || show_deleted || show_others || show_unmerged ||
- show_killed || show_modified || show_resolve_undo ||
- show_valid_bit || show_tag || show_eol))
- die("ls-files --recurse-submodules can only be used in "
- "--cached mode");
+ (show_deleted || show_others || show_unmerged ||
+ show_killed || show_modified || show_resolve_undo))
+ die("ls-files --recurse-submodules unsupported mode");
if (recurse_submodules && error_unmatch)
die("ls-files --recurse-submodules does not support "
"--error-unmatch");
- if (recurse_submodules && argc)
- die("ls-files --recurse-submodules does not support path "
- "arguments");
-
parse_pathspec(&pathspec, 0,
PATHSPEC_PREFER_CWD |
PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
prefix, argv);
- /* Find common prefix for all pathspec's */
- max_prefix = common_prefix(&pathspec);
+ /*
+ * Find common prefix for all pathspec's
+ * This is used as a performance optimization which unfortunately cannot
+ * be done when recursing into submodules
+ */
+ if (recurse_submodules)
+ max_prefix = NULL;
+ else
+ max_prefix = common_prefix(&pathspec);
max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
/* Treat unmatching pathspec elements as errors */
diff --git a/dir.c b/dir.c
index 0ea235f..1afc3ff 100644
--- a/dir.c
+++ b/dir.c
@@ -63,6 +63,35 @@ int fspathncmp(const char *a, const char *b, size_t count)
return ignore_case ? strncasecmp(a, b, count) : strncmp(a, b, count);
}
+/**
+ * Used to perform prefix matching against a pathspec_item for determining if we
+ * should decend into a submodule. This function can result in false positives
+ * since we are only trying to match the 'string' to a prefix of the 'pattern'
+ */
+static int prefix_fnmatch(const struct pathspec_item *item,
+ const char *pattern, const char *string,
+ int prefix)
+{
+ if (prefix > 0) {
+ if (ps_strncmp(item, pattern, string, prefix))
+ return WM_NOMATCH;
+ pattern += prefix;
+ string += prefix;
+ }
+
+ if (item->flags & PATHSPEC_ONESTAR) {
+ return WM_MATCH;
+ } else if (item->magic & PATHSPEC_GLOB) {
+ return wildmatch(pattern, string,
+ WM_PATHNAME |
+ (item->magic & PATHSPEC_ICASE ?
+ WM_CASEFOLD : 0),
+ NULL);
+ }
+
+ return WM_NOMATCH;
+}
+
int git_fnmatch(const struct pathspec_item *item,
const char *pattern, const char *string,
int prefix)
@@ -207,8 +236,9 @@ int within_depth(const char *name, int namelen,
return 1;
}
-#define DO_MATCH_EXCLUDE 1
-#define DO_MATCH_DIRECTORY 2
+#define DO_MATCH_EXCLUDE (1<<0)
+#define DO_MATCH_DIRECTORY (1<<1)
+#define DO_MATCH_SUBMODULE (1<<2)
/*
* Does 'match' match the given name?
@@ -283,6 +313,24 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,
item->nowildcard_len - prefix))
return MATCHED_FNMATCH;
+ /* Perform checks to see if "name" is a super set of the pathspec */
+ if (flags & DO_MATCH_SUBMODULE) {
+ int matched = 0;
+
+ /* Check if the name is a literal prefix of the pathspec */
+ if ((item->match[namelen] == '/') &&
+ !ps_strncmp(item, match, name, namelen)) {
+ matched = MATCHED_RECURSIVELY;
+ /* Check if the name wildmatches to the pathspec */
+ } else if (item->nowildcard_len < item->len &&
+ !prefix_fnmatch(item, match, name,
+ item->nowildcard_len - prefix)) {
+ matched = MATCHED_FNMATCH;
+ }
+
+ return matched;
+ }
+
return 0;
}
@@ -386,6 +434,21 @@ int match_pathspec(const struct pathspec *ps,
return negative ? 0 : positive;
}
+/**
+ * Check if a submodule is a superset of the pathspec
+ */
+int submodule_path_match(const struct pathspec *ps,
+ const char *submodule_name,
+ char *seen)
+{
+ int matched = do_match_pathspec(ps, submodule_name,
+ strlen(submodule_name),
+ 0, seen,
+ DO_MATCH_DIRECTORY |
+ DO_MATCH_SUBMODULE);
+ return matched;
+}
+
int report_path_error(const char *ps_matched,
const struct pathspec *pathspec,
const char *prefix)
diff --git a/dir.h b/dir.h
index da1a858..97c83bb 100644
--- a/dir.h
+++ b/dir.h
@@ -304,6 +304,10 @@ extern int git_fnmatch(const struct pathspec_item *item,
const char *pattern, const char *string,
int prefix);
+extern int submodule_path_match(const struct pathspec *ps,
+ const char *submodule_name,
+ char *seen);
+
static inline int ce_path_match(const struct cache_entry *ce,
const struct pathspec *pathspec,
char *seen)
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index caf3815..977f85c 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -69,9 +69,63 @@ test_expect_success 'ls-files recurses more than 1 level' '
test_cmp expect actual
'
-test_expect_success '--recurse-submodules does not support using path arguments' '
- test_must_fail git ls-files --recurse-submodules b 2>actual &&
- test_i18ngrep "does not support path arguments" actual
+test_expect_success '--recurse-submodules and pathspecs setup' '
+ echo e >submodule/subsub/e.txt &&
+ git -C submodule/subsub add e.txt &&
+ git -C submodule/subsub commit -m "adding e.txt" &&
+ echo f >submodule/f.TXT &&
+ echo g >submodule/g.txt &&
+ git -C submodule add f.TXT g.txt &&
+ git -C submodule commit -m "add f and g" &&
+ echo h >h.txt &&
+ git add h.txt &&
+ git commit -m "add h" &&
+
+ cat >expect <<-\EOF &&
+ .gitmodules
+ a
+ b/b
+ h.txt
+ submodule/.gitmodules
+ submodule/c
+ submodule/f.TXT
+ submodule/g.txt
+ submodule/subsub/d
+ submodule/subsub/e.txt
+ EOF
+
+ git ls-files --recurse-submodules "*" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+ cat >expect <<-\EOF &&
+ h.txt
+ submodule/g.txt
+ submodule/subsub/e.txt
+ EOF
+
+ git ls-files --recurse-submodules "*.txt" >actual &&
+ test_cmp expect actual &&
+
+ cat >expect <<-\EOF &&
+ h.txt
+ submodule/f.TXT
+ submodule/g.txt
+ submodule/subsub/e.txt
+ EOF
+
+ git ls-files --recurse-submodules ":(icase)*.txt" >actual &&
+ test_cmp expect actual &&
+
+ cat >expect <<-\EOF &&
+ h.txt
+ submodule/f.TXT
+ submodule/g.txt
+ EOF
+
+ git ls-files --recurse-submodules ":(icase)*.txt" ":(exclude)submodule/subsub/*" >actual &&
+ test_cmp expect actual
'
test_expect_success '--recurse-submodules does not support --error-unmatch' '
@@ -82,18 +136,14 @@ test_expect_success '--recurse-submodules does not support --error-unmatch' '
test_incompatible_with_recurse_submodules () {
test_expect_success "--recurse-submodules and $1 are incompatible" "
test_must_fail git ls-files --recurse-submodules $1 2>actual &&
- test_i18ngrep 'can only be used in --cached mode' actual
+ test_i18ngrep 'unsupported mode' actual
"
}
-test_incompatible_with_recurse_submodules -v
-test_incompatible_with_recurse_submodules -t
test_incompatible_with_recurse_submodules --deleted
test_incompatible_with_recurse_submodules --modified
test_incompatible_with_recurse_submodules --others
-test_incompatible_with_recurse_submodules --stage
test_incompatible_with_recurse_submodules --killed
test_incompatible_with_recurse_submodules --unmerged
-test_incompatible_with_recurse_submodules --eol
test_done
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: [PATCH] ls-files: add pathspec matching for submodules
From: Brandon Williams @ 2016-09-19 18:35 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Heiko Voigt, Nguyễn Thái Ngọc Duy,
Stefan Beller
In-Reply-To: <xmqqoa3jeo1y.fsf@gitster.mtv.corp.google.com>
I thought as much. Thanks for the quick explanation :)
On Mon, Sep 19, 2016 at 11:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Williams <bmwill@google.com> writes:
>
>> side question if the answer is short: Any reason as to why all of the
>> pathspec matching code lives inside of dir.c and not pathspec.c?
>
> Hysterical Raisins.
>
> A pathspec used to be just a "const char **" in simpler times, which
> was no more elaborate than "argv + i" (i.e. after parsing options
> and revisions out, the remainders are pathspec elements). Major
> part of the matching was done inside dir.c and we haven't had chance
> to look at the larger picture to move the code around.
^ permalink raw reply
* Re: [PATCH] ls-files: add pathspec matching for submodules
From: Junio C Hamano @ 2016-09-19 18:34 UTC (permalink / raw)
To: Brandon Williams
Cc: git, Heiko Voigt, Nguyễn Thái Ngọc Duy,
Stefan Beller
In-Reply-To: <CAKoko1rULusRdcw7_u0QRfn590uTAZyOTk4JS6b8oopaSDLNDQ@mail.gmail.com>
Brandon Williams <bmwill@google.com> writes:
> side question if the answer is short: Any reason as to why all of the
> pathspec matching code lives inside of dir.c and not pathspec.c?
Hysterical Raisins.
A pathspec used to be just a "const char **" in simpler times, which
was no more elaborate than "argv + i" (i.e. after parsing options
and revisions out, the remainders are pathspec elements). Major
part of the matching was done inside dir.c and we haven't had chance
to look at the larger picture to move the code around.
^ permalink raw reply
* Re: [PATCH] ls-files: add pathspec matching for submodules
From: Brandon Williams @ 2016-09-19 18:30 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Heiko Voigt, Nguyễn Thái Ngọc Duy,
Stefan Beller
In-Reply-To: <xmqqshsveomv.fsf@gitster.mtv.corp.google.com>
side question if the answer is short: Any reason as to why all of the
pathspec matching code lives inside of dir.c and not pathspec.c?
On Mon, Sep 19, 2016 at 11:22 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Williams <bmwill@google.com> writes:
>
>> Yes in that case it wouldn't have passed ps_strncmp()...but we should have never
>> made it there in the first place due to a piece of logic in match_pathspec_item:
>
> Ah, OK.
>
> Thanks for clarifying.
^ permalink raw reply
* Re: [PATCH] ls-files: add pathspec matching for submodules
From: Junio C Hamano @ 2016-09-19 18:22 UTC (permalink / raw)
To: Brandon Williams
Cc: git, Heiko Voigt, Nguyễn Thái Ngọc Duy,
Stefan Beller
In-Reply-To: <CAKoko1oSvBF3=-hvSGVMQYpZkUvnwAEYg=U-g8927F6dt4Q8mA@mail.gmail.com>
Brandon Williams <bmwill@google.com> writes:
> Yes in that case it wouldn't have passed ps_strncmp()...but we should have never
> made it there in the first place due to a piece of logic in match_pathspec_item:
Ah, OK.
Thanks for clarifying.
^ permalink raw reply
* Re: [PATCH] ls-files: add pathspec matching for submodules
From: Brandon Williams @ 2016-09-19 18:20 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Heiko Voigt, Nguyễn Thái Ngọc Duy,
Stefan Beller
In-Reply-To: <xmqq1t0fg417.fsf@gitster.mtv.corp.google.com>
On Mon, Sep 19, 2016 at 11:04 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Williams <bmwill@google.com> writes:
>
>>> Again, what do we have in "name" and "item" at this point? If we
>>> have a submodule at "sub/" and we are checking a pathspec element
>>> "sub/dir1/*", what is the non-wildcard part of the pathspec and what
>>> is the "string"? Aren't they "sub/dir1/" and "sub/" respectively,
>>> which would not pass ps_strncmp() and produce a (false) negative?
>>
>> item will be the pathspec_item struct that we are trying to match against.
>
> ... which would mean "sub/dir1/" in the above example (which is
> followed by '*' that is wildcard).
>
>> name will be the file we are trying to match, which should already have the
>> 'prefix' cut off (this is the prefix that is used as an optimization
>> in the common
>> case, which isn't used in the submodule case).
>
> ... which would be "sub/" in the above example, because we disable
> the common-prefix optimization.
>
> So in short, the answer to the last questions in the first quoted
> paragraph are yes, yes, and "no they do not pass ps_strncmp()"?
Yes in that case it wouldn't have passed ps_strncmp()...but we should have never
made it there in the first place due to a piece of logic in match_pathspec_item:
@@ -283,6 +308,24 @@ static int match_pathspec_item(const struct
pathspec_item *item, int prefix,
item->nowildcard_len - prefix))
return MATCHED_FNMATCH;
+ /* Perform checks to see if "name" is a super set of the pathspec */
+ if (flags & DO_MATCH_SUBMODULE) {
+ int matched = 0;
+
+ /* Check if the name is a literal prefix of the pathspec */
+ if ((item->match[namelen] == '/') &&
+ !ps_strncmp(item, match, name, namelen)) {
+ matched = MATCHED_RECURSIVELY;
+ /* Check if the name wildmatches to the pathspec */
+ } else if (item->nowildcard_len < item->len &&
+ !prefix_fnmatch(item, match, name,
+ item->nowildcard_len - prefix)) {
+ matched = MATCHED_FNMATCH;
+ }
+
+ return matched;
+ }
Perhaps the call structure and code organization could be changes a bit to make
a little more sense.
^ permalink raw reply
* Re: [PATCH] ls-files: add pathspec matching for submodules
From: Junio C Hamano @ 2016-09-19 18:18 UTC (permalink / raw)
To: Brandon Williams; +Cc: git
In-Reply-To: <1474073981-96620-1-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> static void show_ce_entry(const char *tag, const struct cache_entry *ce)
> {
> + struct strbuf name = STRBUF_INIT;
> int len = max_prefix_len;
> + if (submodule_prefix)
> + strbuf_addstr(&name, submodule_prefix);
> + strbuf_addstr(&name, ce->name);
Continuing with the previous review, which concentrated on what
happens in the superproject; let's see what happens in the recursive
invocation in a submodule.
So a recursively spawned "ls-files --submodule-prefix=sub/" finds
a path in the index PATH and forms "sub/PATH" in "name". From here
on, where we used to match pathspec against ce->name, we would be
matching it against name->buf.
> + if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&
> + submodule_path_match(&pathspec, name.buf, ps_matched)) {
> show_gitlink(ce);
This is primarily what happens in the superproject to decide if the
submodule is worth showing. When we are in a submodule, we can
descend into subsubmodule (if our ls-files run in the superproject
passed --recurse-submodule down) from here.
> + } else if (match_pathspec(&pathspec, name.buf, name.len,
> + len, ps_matched,
> + S_ISDIR(ce->ce_mode) ||
> + S_ISGITLINK(ce->ce_mode))) {
This is interesting bit to see what happens in the recursive
invocation. It uses the usual match_pathspec(), as we want to be
precise and correct, iow, we do not want to use DO_MATCH_SUBMODULE,
aka "it might be worth descending into submodule".
> + if (tag && *tag && show_valid_bit &&
> + (ce->ce_flags & CE_VALID)) {
> +...
> + }
> + write_eolinfo(ce, ce->name);
> + write_name(ce->name);
The prefixing is taken care of by write_name(), so it is correct to
use ce->name here.
> ...
> + strbuf_release(&name);
> }
OK, everything I saw so far for the recursive invocation here makes
sense.
Thanks.
^ permalink raw reply
* RE: Switching branches not working in a cloned repo
From: Paul Williamson @ 2016-09-19 18:01 UTC (permalink / raw)
To: Philip Oakley, git@vger.kernel.org
In-Reply-To: <3999677D12354A3CB2035E932353981A@PhilipOakley>
-----Original Message-----
From: Philip Oakley [mailto:philipoakley@iee.org]
>Have you tried `git ls-remote` ?
>The `branch -r` just lists the local 'rtb's (IIUC).
Nice, I didn't know about that command - I tried it though and it does list the remote branches correctly. I checked the commit refs given and it tallies with the refs at the gitolite server end.
>It could be someone has accidently pruned or deleted that branch at the remote.
I don't think so, as I say it's a gitolite server, so usually nobody is on it. We just push to it and set up post-receive hooks for if updates need to be pushed on to a deployment server.
Do you know of ways to check if anything is corrupted within the repo? I ran git fsck, but that didn't show up anything.
>What version are you (they) on?
1.9.1 at both ends (my laptop and our gitolite server)
> Gmane had to quit. Try http://public-inbox.org/git (see the help link)
Ah! Thanks. I tried searching but so far only turned up this thread.
> philip
Thanks for your reply, Philip.
Paul
^ permalink raw reply
* Re: [PATCH] ls-files: add pathspec matching for submodules
From: Junio C Hamano @ 2016-09-19 18:04 UTC (permalink / raw)
To: Brandon Williams
Cc: git, Heiko Voigt, Nguyễn Thái Ngọc Duy,
Stefan Beller
In-Reply-To: <CAKoko1r_WATxJzxQrQW2VBkhuKquv=yQv6sB_eCMgH6qavS__Q@mail.gmail.com>
Brandon Williams <bmwill@google.com> writes:
>> Again, what do we have in "name" and "item" at this point? If we
>> have a submodule at "sub/" and we are checking a pathspec element
>> "sub/dir1/*", what is the non-wildcard part of the pathspec and what
>> is the "string"? Aren't they "sub/dir1/" and "sub/" respectively,
>> which would not pass ps_strncmp() and produce a (false) negative?
>
> item will be the pathspec_item struct that we are trying to match against.
... which would mean "sub/dir1/" in the above example (which is
followed by '*' that is wildcard).
> name will be the file we are trying to match, which should already have the
> 'prefix' cut off (this is the prefix that is used as an optimization
> in the common
> case, which isn't used in the submodule case).
... which would be "sub/" in the above example, because we disable
the common-prefix optimization.
So in short, the answer to the last questions in the first quoted
paragraph are yes, yes, and "no they do not pass ps_strncmp()"?
>> I am starting to have a feeling that the best we can do in this
>> function safely is to see if prefix (i.e. the constant part of the
>> pathspec before the first wildcard) is long enough to cover the
>> "name" and if "name" part [matches or does not match] ...
>> If these two checks cannot decide, we may have to be pessimistic and
>> say "it may match; we don't know until we descend into it".
>> ...
>> So I would think we'd be in the business of counting slashes in the
>> name (called "string" in this function) and the pathspec, while
>> noticing '*' and '**' in the latter, and we may be able to be more
>> precise, but I am not sure how complex the end result would become.
>
> I agree, I'm not too sure how much more complex the logic would need
> to be to handle
> all matters of wildcard characters. We could initially be more
> lenient on what qualifies as
> a match and then later (or in the near future) revisit the wildmatch
> function (which is complex)
> and see if we can add better matching capabilities more suited for
> submodules while at the
> same time fixing that bug discussed above.
I think it is reasonable to start a function that is meant to never
have false negatives pessimistic and return "might match" from it
when in doubt.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/6] i18n: commit: mark message for translation
From: Junio C Hamano @ 2016-09-19 17:54 UTC (permalink / raw)
To: Vasco Almeida
Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason,
Jean-Noël AVILA
In-Reply-To: <1474290501-2743-1-git-send-email-vascomalmeida@sapo.pt>
I am responding to 1/6, as the series lacked a cover letter, but all
of them looked good.
Thanks.
^ permalink raw reply
* Re: [PATCH v2] format-patch: Add --rfc for the common case of [RFC PATCH]
From: Junio C Hamano @ 2016-09-19 17:49 UTC (permalink / raw)
To: Andrew Donnellan; +Cc: Josh Triplett, git, Jeff King
In-Reply-To: <3df15bbb-7eac-86ec-2ccb-74a973482e8c@au1.ibm.com>
Andrew Donnellan <andrew.donnellan@au1.ibm.com> writes:
> Sounds good to me. Agreed that "RFC" is essentially the only prefix
> other than "PATCH" that I see, at least in the kernel.
Around here I think we saw WIP too, and that makes me lean towards
Peff's earlier suggestion to allow an end-user supplied string in
front of PATCH, i.e. "-P RFC" => "--subject-prefix='RFC PATCH'",
even though I understand that those who _ONLY_ care about RFC would
prefer --rfc (5 keystrokes) over "-P RFC" (6 keystrokes).
>> +--rfc::
>> + Alias for `--subject-prefix="RFC PATCH"`. Use this when
>> + sending an experimental patch for discussion rather than
>> + application.
>
> Perhaps mention the phrase "Request For Comment" for the benefit of
> those who aren't familiar ...
Good point.
^ permalink raw reply
* Re: [Bug] Custom git-dir directory shouldn't be listed as “untracked”
From: Junio C Hamano @ 2016-09-19 17:45 UTC (permalink / raw)
To: Nicolas Cuillery; +Cc: git
In-Reply-To: <CACmQg1hE=ytaatDfUJLkhL0p5c43wZZvgt+8pc5zoo0YFdQw6A@mail.gmail.com>
Nicolas Cuillery <nicolas.cuillery@gmail.com> writes:
> When using the default directory ".git", it logically doesn't appear
> in the "git status" command's output. Don't you think it should be the
> same when using a custom dir name ?
Not really.
GIT_DIR=<there> mechanism was never meant to be used to name a
directory that sitsinside your working tree (an exception is if it
is actually ".git" at the top).
^ 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