* Re: [PATCH 1/2] ls-files: adding support for submodules
From: Junio C Hamano @ 2016-09-27 22:23 UTC (permalink / raw)
To: Brandon Williams; +Cc: Jeff King, git
In-Reply-To: <20160927220955.GA38615@google.com>
Brandon Williams <bmwill@google.com> writes:
>> If you are in a subdirectory of your superproject, say, a/,
>>
>> cd a && git ls-files --recurse-submodules -- "b*"
>>
>> I would expect we would recurse into the submodule at "a/b" and find
>> "b/file-at-top-of-B". What does the internal invocation to do so
>> would look like? I would think "git -C b --super=b ls-files" that
>> is run from "a".
Actually, the internal invocation may have to be
$ git --super=a/b ls-files -- "a/b*"
if the desired overall output needs to be in the "--full-name" mode.
That is, the top-level recursive one may be
cd a && git ls-files --recurse-submodules --full-name -- "b*"
This top-level "ls-files" will have "prefix" set to "a/". Because
it is run in the "--full-name" mode, after finding that the
submodule at "a/b" matches the given pathspec and deciding to
recurse into it, it needs to arrange that paths stored in the index
of the submodule are prefixed with "a/b/", not with "b/", when
shown.
^ permalink raw reply
* Re: [PATCH v2] rev-list-options: clarify the usage of --reverse
From: Philip Oakley @ 2016-09-27 22:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Pranit Bauva, git
In-Reply-To: <xmqqoa39kn57.fsf@gitster.mtv.corp.google.com>
From: "Junio C Hamano" <gitster@pobox.com>
> "Philip Oakley" <philipoakley@iee.org> writes:
>
>> micro-nit: 'first' and 'last' can be tricky to distinguish for lists...
>
> Let's do this then.
Looks good to me. Thanks. -- Philip
>
> -- >8 --
> From: Pranit Bauva <pranit.bauva@gmail.com>
> Date: Tue, 27 Sep 2016 20:44:09 +0000
> Subject: [PATCH] rev-list-options: clarify the usage of --reverse
>
> Users often wonder if the oldest or the newest n commits are shown
> by `log -n --reverse`. Clarify that --reverse kicks in only after
> deciding which commits are to be shown to unconfuse them.
>
> Reported-by: Ruediger Meier <sweet_f_a@gmx.de>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> Documentation/rev-list-options.txt | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/rev-list-options.txt
> b/Documentation/rev-list-options.txt
> index 4f009d4..f8946d4 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -638,8 +638,9 @@ avoid showing the commits from two parallel
> development track mixed
> together.
>
> --reverse::
> - Output the commits in reverse order.
> - Cannot be combined with `--walk-reflogs`.
> + Output the commits chosen to be shown (see Commit Limiting
> + section above) in reverse order. Cannot be combined with
> + `--walk-reflogs`.
>
> Object Traversal
> ~~~~~~~~~~~~~~~~
> --
> 2.10.0-561-g98a6b79
>
>
^ permalink raw reply
* Re: [PATCH v2] rev-list-options: clarify the usage of --reverse
From: Junio C Hamano @ 2016-09-27 22:12 UTC (permalink / raw)
To: Philip Oakley; +Cc: Pranit Bauva, git
In-Reply-To: <A9427F4400044104B47378E024C7C2A4@PhilipOakley>
"Philip Oakley" <philipoakley@iee.org> writes:
> micro-nit: 'first' and 'last' can be tricky to distinguish for lists...
Let's do this then.
-- >8 --
From: Pranit Bauva <pranit.bauva@gmail.com>
Date: Tue, 27 Sep 2016 20:44:09 +0000
Subject: [PATCH] rev-list-options: clarify the usage of --reverse
Users often wonder if the oldest or the newest n commits are shown
by `log -n --reverse`. Clarify that --reverse kicks in only after
deciding which commits are to be shown to unconfuse them.
Reported-by: Ruediger Meier <sweet_f_a@gmx.de>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/rev-list-options.txt | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 4f009d4..f8946d4 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -638,8 +638,9 @@ avoid showing the commits from two parallel development track mixed
together.
--reverse::
- Output the commits in reverse order.
- Cannot be combined with `--walk-reflogs`.
+ Output the commits chosen to be shown (see Commit Limiting
+ section above) in reverse order. Cannot be combined with
+ `--walk-reflogs`.
Object Traversal
~~~~~~~~~~~~~~~~
--
2.10.0-561-g98a6b79
^ permalink raw reply related
* Re: [PATCH 1/2] ls-files: adding support for submodules
From: Brandon Williams @ 2016-09-27 22:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <xmqqwphxknoj.fsf@gitster.mtv.corp.google.com>
On 09/27, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
>
> > Well maybe...I don't really know much about how the prefix interacts in
> > every scenario but would what you describe still work if we are in a sub
> > dir of the superproject (which contains other directorys and perhaps a
> > submodule) and execute a --recurse-submodules command in the
> > subdirectory? I suspect we don't want to force users to be in the root
> > directory of the project in order to use --recurse-submodules.
>
> You need to remember "must be at the top" is relevant only to the
> command that is invoked with --super-prefix, not the recursive one
> that drives such a process.
>
> Suppose your superproject is organized like so:
>
> - file-at-top
> a/file-in-A
> a/b (submodule)
> a/b/file-at-top-of-B
> c/ (submodule)
> c/file-at-top-of-C
>
> If you are in a subdirectory of your superproject, say, a/,
>
> cd a && git ls-files --recurse-submodules -- "b*"
>
> I would expect we would recurse into the submodule at "a/b" and find
> "b/file-at-top-of-B". What does the internal invocation to do so
> would look like? I would think "git -C b --super=b ls-files" that
> is run from "a".
>
> Your code would is already prepared to find "file-at-top-of-B" in
> the index of the submodule, prepend "b/" to it and report the result
> as "b/file-at-top-of-B" when such a call is made, I think.
>
> Now, can you refer to c/ and c/file-at-top-of-C while sitting at a/?
>
> cd a && git ls-files --recurse-submodules -- "../c*"
>
> would be the top-level invocation. This would iterate over the
> index of the superproject, trying to find what matches "c*" (or,
> "../c*" relative to "a" i.e. where you are), find that 'c' that is a
> submodule, and invoke "git -C ../c --super=../c ls-files"
> internally, I would imagine. I think your code is prepared to
> accept this case as well.
>
> In any case, the "must be at the top" does not come into the picture
> at all for the end-user interaction, i.e. invocation of the command
> that is told to recurse into submodules, so we'd be OK.
Thanks for the clear explination that makes sense.
Also, --super-prefix as a name is growing on me :)
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 00/11] Resumable clone
From: Junio C Hamano @ 2016-09-27 22:07 UTC (permalink / raw)
To: Eric Wong; +Cc: Kevin Wern, git
In-Reply-To: <20160927215143.GA32622@starla>
Eric Wong <e@80x24.org> writes:
>> [primeclone]
>> url = http://location/pack-$NAME.pack
>> filetype = pack
>
> If unconfigured, I wonder if a primeclone pack can be inferred by
> the existence of a pack bitmap (or merely being the biggest+oldest
> pack for dumb HTTP).
That would probably be a nice heuristics but it is unclear who
should find that out at runtime. The downloading side would not
have a visiblity into directory listing.
>> git clone --resume <resumable_work_or_git_dir>
>
> I think calling "git fetch" should resume, actually.
> It would reduce the learning curve and seems natural to me:
> "fetch" is jabout grabbing whatever else appeared since the
> last clone/fetch happened.
I hate say this but it sounds to me like a terrible idea. At that
point when you need to resume, there is not even ref for "fetch" to
base its incremental work off of. It is better to keep the knowledge
of this "priming" dance inside "clone". Hopefully the original "clone"
whose connection was disconnected in the middle would automatically
attempt resuming and "clone --resume" would not be as often as needed.
^ permalink raw reply
* Re: [PATCH 1/2] ls-files: adding support for submodules
From: Junio C Hamano @ 2016-09-27 22:01 UTC (permalink / raw)
To: Brandon Williams; +Cc: Jeff King, git
In-Reply-To: <20160927214854.GA180705@google.com>
Brandon Williams <bmwill@google.com> writes:
> Well maybe...I don't really know much about how the prefix interacts in
> every scenario but would what you describe still work if we are in a sub
> dir of the superproject (which contains other directorys and perhaps a
> submodule) and execute a --recurse-submodules command in the
> subdirectory? I suspect we don't want to force users to be in the root
> directory of the project in order to use --recurse-submodules.
You need to remember "must be at the top" is relevant only to the
command that is invoked with --super-prefix, not the recursive one
that drives such a process.
Suppose your superproject is organized like so:
- file-at-top
a/file-in-A
a/b (submodule)
a/b/file-at-top-of-B
c/ (submodule)
c/file-at-top-of-C
If you are in a subdirectory of your superproject, say, a/,
cd a && git ls-files --recurse-submodules -- "b*"
I would expect we would recurse into the submodule at "a/b" and find
"b/file-at-top-of-B". What does the internal invocation to do so
would look like? I would think "git -C b --super=b ls-files" that
is run from "a".
Your code would is already prepared to find "file-at-top-of-B" in
the index of the submodule, prepend "b/" to it and report the result
as "b/file-at-top-of-B" when such a call is made, I think.
Now, can you refer to c/ and c/file-at-top-of-C while sitting at a/?
cd a && git ls-files --recurse-submodules -- "../c*"
would be the top-level invocation. This would iterate over the
index of the superproject, trying to find what matches "c*" (or,
"../c*" relative to "a" i.e. where you are), find that 'c' that is a
submodule, and invoke "git -C ../c --super=../c ls-files"
internally, I would imagine. I think your code is prepared to
accept this case as well.
In any case, the "must be at the top" does not come into the picture
at all for the end-user interaction, i.e. invocation of the command
that is told to recurse into submodules, so we'd be OK.
^ permalink raw reply
* Re: [PATCH 00/11] Resumable clone
From: Eric Wong @ 2016-09-27 21:51 UTC (permalink / raw)
To: Kevin Wern; +Cc: git
In-Reply-To: <1473984742-12516-1-git-send-email-kevin.m.wern@gmail.com>
Kevin Wern <kevin.m.wern@gmail.com> wrote:
> Hey, all,
>
> It's been a while (sent a very short patch in May), but I've
> still been working on the resumable clone feature and checking up on
> the mailing list for any updates. After submitting the prime-clone
> service alone, I figured implementing the whole thing would be the best
> way to understand the full scope of the problem (this is my first real
> contribution here, and learning while working on such an involved
> feature has not been easy).
Thank you for working on this. I'm hugely interested in this
feature as both a cheapskate sysadmin and as a client with
unreliable connectivity (and I've barely been connected this month).
> This is a functional implementation handling a direct http/ftp URI to a
> single, fully connected packfile (i.e. the link is a direct path to the
> file, not a prefix or guess). My hope is that this acts as a bare
> minimum cross-section spanning the full requirments that can expand in
> width as more cases are added (.info file, split bundle, daemon
> download service). This is certainly not perfect, but I think it at
> least prototypes each component involved in the workflow.
>
> This patch series is based on jc/bundle, because the logic to find the
> tips of a pack's history already exists there (I call index-pack
> --clone-bundle on the downloaded file, and read the file to write the
> references to a temporary directory). If I need to re-implement this
> logic or base it on another branch, let me know. For ease of pulling
> and testing, I included the branch here:
>
> https://github.com/kevinwern/git/tree/feature/prime-clone
Am I correct this imposes no additional storage burden for servers?
(unlike the current .bundle dance used by kernel.org:
https://www.kernel.org/cloning-linux-from-a-bundle.html )
That would be great!
> Although there are a few changes internally from the last patch,
> the "alternate resource" url to download is configured on the
> server side in exactly the same way:
>
> [primeclone]
> url = http://location/pack-$NAME.pack
> filetype = pack
If unconfigured, I wonder if a primeclone pack can be inferred by
the existence of a pack bitmap (or merely being the biggest+oldest
pack for dumb HTTP).
> The prime-clone service simply outputs the components as:
>
> ####url filetype
> 0000
>
> On the client side, the transport_prime_clone and
> transport_download_primer APIs are built to be more robust (i.e. read
> messages without dying due to protocol errors), so that git clone can
> always try them without being dependent on the capability output of
> git-upload-pack. transport_download_primer is dependent on the success
> of transport_prime_clone, but transport_prime_clone is always run on an
> initial clone. Part of achieving this robustness involves adding
> *_gentle functions to pkt_line, so that prime_clone can fail silently
> without dying.
>
> The transport_download_primer function uses a resumable download,
> which is applicable to both automatic and manual resuming. Automatic
> is programmatically reconnecting to the resource after being
> interrupted (up to a set number of times). Manual is using a newly
> taught --resume option on the command line:
>
> git clone --resume <resumable_work_or_git_dir>
I think calling "git fetch" should resume, actually.
It would reduce the learning curve and seems natural to me:
"fetch" is jabout grabbing whatever else appeared since the
last clone/fetch happened.
> Right now, a manually resumable directory is left behind only if the
> *client* is interrupted while a new junk mode, JUNK_LEAVE_RESUMABLE,
> is set (right before the download). For an initial clone, if the
> connection fails after automatic resuming, the client erases the
> partial resources and falls through to a normal clone. However, once a
> resumable directory is left behind by the program, it is NEVER
> deleted/abandoned after it is continued with --resume.
I'm not sure if erasing partial resources should ever be done
automatically. Perhaps a note to the user explaining the
situation and potential ways to correct/resume it.
> I think determining when a resource is "unsalvageable" should be more
> nuanced. Especially in a case where a connection is perpetually poor
> and the user wishes to resume over a long period of time. The timeout
> logic itself *definitely* needs more nuance than "repeat 5 times", such
> as expanding wait times and using earlier successes when deciding to
> try again. Right now, I think the most important part of this patch is
> that these two paths (falling through after a failed download, exiting
> to be manually resumed later) exist.
>
> Off the top of my head, outstanding issues/TODOs inlcude:
> - The above issue of determining when to fall through, when to
> reattempt, and when to write the resumable info and exit
> in git clone.
My current (initial) reaction is: you're overthinking this.
I think it's less surprising to a user to always write resumable
info and let them know how to resume (or abort); rather than
trying to second-guess their intent.
Going by the zero-one-infinity rule, I'd probably attempt an
auto-retry once on socket errors before saving state and bailing
with instructions on how to resume.
If they hit Ctrl-C manually, then just tell them they can
either resume or "rm -r" the directory.
> - Creating git-daemon service to download a resumable resource.
> Pretty straightforward, I think, especially if
> http.getanyfile already exists. This falls more under
> "haven't gotten to yet" than dilemma.
I think this could be handled natively by git-daemon for
trickling data to slow clients in the existing event loop (and
expanded to use epoll/kqueue). Similar to how X-Sendfile works
with (Apache|lighttpd) or X-Accel in nginx.
This would be cheaper than wasting a process (or thread) to
trickle to low-bandwidth clients. But this may be an
optimization we defer until we've ironed out other parts.
> - Logic for git clone to determine when a full clone would
> be superior, such as when a clone is local or a reference is
> given.
> - Configuring prime-clone for multiple resources, in two
> dimensions: (a) resources to choose from (e.g. fall back to
> a second resource if the first one doesn't work) and (b)
> resources to be downloaded together or in sequence (e.g.
> download http://host/this, then http://host/that). Maybe
> prime-clone could also handle client preferences in terms of
> filetype or protocol. For this, I just have to re-read a few
> discussions about the filetypes we use to see if there are
> any outliers that aren't representable in this way. I think
> this is another "haven't gotten to yet".
Perhaps using the existing http-alternates (and automatic
primeclone pack inference I wrote about above) can be done.
<snip>
> - Creating the logic to guess a packfile, and append that to a
> prefix specified by the admin. Additionally, allowing the
> admin to use a custom script to use their own logic to
> output the URL.
Yes :) Though I'm not sure if the custom script is necessary.
^ permalink raw reply
* Re: [PATCH v2] rev-list-options: clarify the usage of --reverse
From: Philip Oakley @ 2016-09-27 21:51 UTC (permalink / raw)
To: Pranit Bauva, git
In-Reply-To: <010201576d63f422-abe45938-0da1-4fc0-b0e7-3e552b59d10a-000000@eu-west-1.amazonses.com>
From: "Pranit Bauva" <pranit.bauva@gmail.com>
> Specify even more clearly that --reverse works only with the commits
> which are chosen to be shown so as to eliminate the confusion as to
> whether the first n or the last n commits with be shown when used
hi Pranit,
micro-nit: 'first' and 'last' can be tricky to distinguish for lists (e.g.
of commits) that can be considered from both ends with equal ease (people
are so easily confused ;-). In such cases its always worth stating the sort
order mechanism (within the appropriate sentence). However, in this case the
clarification is to clearly point elsewhere, so I'm OK with the patch.
> with `-n --reverse`.
>
> Reported-by: Ruediger Meier <sweet_f_a@gmx.de>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> Documentation/rev-list-options.txt | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/rev-list-options.txt
> b/Documentation/rev-list-options.txt
> index 7e462d3..5da7cf5 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -657,8 +657,9 @@ avoid showing the commits from two parallel
> development track mixed
> together.
>
> --reverse::
> - Output the commits in reverse order.
> - Cannot be combined with `--walk-reflogs`.
> + Output the commits chosen to be shown (see Commit Limiting
> + section above) in reverse order. Cannot be combined with
> + `--walk-reflogs`.
>
> Object Traversal
> ~~~~~~~~~~~~~~~~
>
> --
> https://github.com/git/git/pull/296
>
--
Philip
^ permalink raw reply
* Re: [PATCH 1/2] ls-files: adding support for submodules
From: Brandon Williams @ 2016-09-27 21:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <xmqq60phm39w.fsf@gitster.mtv.corp.google.com>
On 09/27, Junio C Hamano wrote:
> > What we internally call "prefix" and "--submodule-prefix" is closely
> > related in that they both interact with pathspecs. "prefix" gets
> > prepended to elements of an end-user supplied pathspec before a
> > full-path-in-the-repository (i.e. a path in the index and a path
> > relative from the top of the working tree) is matched against them.
>
> In a sense, this new thing is a superset of the existing GIT_PREFIX,
> which gives the current working directory from the end user's point
> of view relative to the actual current working directory. The "git"
> wrapper, when running a "git thing" from a subdirectory of a working
> tree, chdir(2)s up to the top-level before spawning the "thing"
> subcommand that is not built-in or a third-party "git-thing" binary
> on user's $PATH; so "git-thing" binary will be told via $GIT_PREFIX
> relative to what directory path-like things the end user supplied to
> the command (e.g. "-o outfile" argument and pathspecs) need to be
> interpreted. The new thing can override the $GIT_PREFIX to allow
> path-like things to be interpreted relative to somewhere _above_ the
> top-level of the working tree.
>
> I am tempted to suggest GIT_SUPER_PREFIX, 50% because I cannot think
> of any better word, 40% because I think it actually makes sense (in
> the sense that this comes _above_ it, hence "super", and also in the
> sense that this is something your "super" project would give you),
> and 10% because I hope that being ridiculous would nudge people to
> come up with a better alternative ;-)
haha, yeah it seems difficult to come up with a name that is going to
mean the same thing in 5 years from now. At least GIT_SUPER_PREFIX
makes senese in that it is a path from the superproject down to the
submodule.
> > * It is unclear how this should interact with commands that are run
> > in a subdirectory of the working tree. E.g. what should the
> > prefix and the pathspec look like if the command in the above
> > example is started in w/git.git/Documentation subdirectory, i.e.
> >
> > $ cd ~
> > $ git -C w/git.git/Documentation ls-files \
> > --submodule-prefix=??????? -- '???????' |
> > xargs ls -1 -l
> >
> > Should we error out if we are not at the top of the working tree
> > when --submodule-prefix is given?
>
> ... the answer to this question becomes clear. It is not possible
> to _be_ in a subdirectory "Documentation" of this working tree and
> in a directory "~" above this working tree at the same time, so we
> simply should forbid running the command from anywhere other than
> the top of the working tree (i.e. the internal "prefix" and
> GIT_PREFIX must be empty) when the super-prefix is set by erroring
> it out. When we realize that "prefix" adds to the paths that are
> supplied by the user (e.g. when the user says Makefile while in t/
> subdirectory, i.e. GIT_PREFIX=t/, s/he means t/Makefile), but this
> new thing subtracts from the paths given by the user (e.g. when the
> user gives a pathspec 'w/git.git/D*' while setting the super thing
> as w/git.git, because s/he is at ~/, the pathspec matcher
> conceptually subtracts w/git.git/ from the pathspec before matching
> them against the paths it finds in the index), it becomes clear that
> giving both at the same time is awkward and not very useful.
Well maybe...I don't really know much about how the prefix interacts in
every scenario but would what you describe still work if we are in a sub
dir of the superproject (which contains other directorys and perhaps a
submodule) and execute a --recurse-submodules command in the
subdirectory? I suspect we don't want to force users to be in the root
directory of the project in order to use --recurse-submodules.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v4 1/2] tree-walk: be more specific about corrupt tree errors
From: Junio C Hamano @ 2016-09-27 21:45 UTC (permalink / raw)
To: David Turner; +Cc: git, peff
In-Reply-To: <1475009991-16368-1-git-send-email-dturner@twosigma.com>
Thanks, will queue both.
^ permalink raw reply
* Re: [PATCH 1/2] ls-files: adding support for submodules
From: Junio C Hamano @ 2016-09-27 21:38 UTC (permalink / raw)
To: Brandon Williams; +Cc: Jeff King, git
In-Reply-To: <xmqq7fa36bwm.fsf@gitster.mtv.corp.google.com>
As this is relevant to what to call the prefix thing that is passed
down to an internal re-invocation of ls-files and how to explain it
to end-users...
Junio C Hamano <gitster@pobox.com> writes:
> I agree that this is not specific to submodules; this is closely
> related to what we internally call "prefix", but is different.
>
> In any case, I would strongly recommend against exposing this (or
> anything for that matter) "--prefix" to the end-user,...
I have a slight suspicion that this was what made you say earlier
that I was against exposing this at all to end users, but what I
meant was a plain boring "prefix" is a bad name and nothing more
than that.
> What we internally call "prefix" and "--submodule-prefix" is closely
> related in that they both interact with pathspecs. "prefix" gets
> prepended to elements of an end-user supplied pathspec before a
> full-path-in-the-repository (i.e. a path in the index and a path
> relative from the top of the working tree) is matched against them.
In a sense, this new thing is a superset of the existing GIT_PREFIX,
which gives the current working directory from the end user's point
of view relative to the actual current working directory. The "git"
wrapper, when running a "git thing" from a subdirectory of a working
tree, chdir(2)s up to the top-level before spawning the "thing"
subcommand that is not built-in or a third-party "git-thing" binary
on user's $PATH; so "git-thing" binary will be told via $GIT_PREFIX
relative to what directory path-like things the end user supplied to
the command (e.g. "-o outfile" argument and pathspecs) need to be
interpreted. The new thing can override the $GIT_PREFIX to allow
path-like things to be interpreted relative to somewhere _above_ the
top-level of the working tree.
I am tempted to suggest GIT_SUPER_PREFIX, 50% because I cannot think
of any better word, 40% because I think it actually makes sense (in
the sense that this comes _above_ it, hence "super", and also in the
sense that this is something your "super" project would give you),
and 10% because I hope that being ridiculous would nudge people to
come up with a better alternative ;-)
And from that point of view ...
> * It is unclear how this should interact with commands that are run
> in a subdirectory of the working tree. E.g. what should the
> prefix and the pathspec look like if the command in the above
> example is started in w/git.git/Documentation subdirectory, i.e.
>
> $ cd ~
> $ git -C w/git.git/Documentation ls-files \
> --submodule-prefix=??????? -- '???????' |
> xargs ls -1 -l
>
> Should we error out if we are not at the top of the working tree
> when --submodule-prefix is given?
... the answer to this question becomes clear. It is not possible
to _be_ in a subdirectory "Documentation" of this working tree and
in a directory "~" above this working tree at the same time, so we
simply should forbid running the command from anywhere other than
the top of the working tree (i.e. the internal "prefix" and
GIT_PREFIX must be empty) when the super-prefix is set by erroring
it out. When we realize that "prefix" adds to the paths that are
supplied by the user (e.g. when the user says Makefile while in t/
subdirectory, i.e. GIT_PREFIX=t/, s/he means t/Makefile), but this
new thing subtracts from the paths given by the user (e.g. when the
user gives a pathspec 'w/git.git/D*' while setting the super thing
as w/git.git, because s/he is at ~/, the pathspec matcher
conceptually subtracts w/git.git/ from the pathspec before matching
them against the paths it finds in the index), it becomes clear that
giving both at the same time is awkward and not very useful.
^ permalink raw reply
* Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules
From: Junio C Hamano @ 2016-09-27 20:59 UTC (permalink / raw)
To: Brandon Williams; +Cc: git
In-Reply-To: <20160927204440.GE32565@google.com>
Brandon Williams <bmwill@google.com> writes:
> Oh there is a separate if gaurd for pathspecs which is introduced in 2/4
> and then removed once pathspec support has been added in 4/4.
Thanks; I missed to spot that when I wrote the message you are
responding to, but it indeed is there ;-)
^ permalink raw reply
* [PATCH v4 1/2] tree-walk: be more specific about corrupt tree errors
From: David Turner @ 2016-09-27 20:59 UTC (permalink / raw)
To: git, peff; +Cc: David Turner
From: Jeff King <peff@peff.net>
When the tree-walker runs into an error, it just calls
die(), and the message is always "corrupt tree file".
However, we are actually covering several cases here; let's
give the user a hint about what happened.
Let's also avoid using the word "corrupt", which makes it
seem like the data bit-rotted on disk. Our sha1 check would
already have found that. These errors are ones of data that
is malformed in the first place.
Signed-off-by: David Turner <dturner@twosigma.com>
Signed-off-by: Jeff King <peff@peff.net>
---
t/t1007-hash-object.sh | 25 +++++++++++++++++++++++--
tree-walk.c | 12 +++++++-----
2 files changed, 30 insertions(+), 7 deletions(-)
diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh
index acca9ac..c5245c5 100755
--- a/t/t1007-hash-object.sh
+++ b/t/t1007-hash-object.sh
@@ -183,9 +183,30 @@ for args in "-w --stdin-paths" "--stdin-paths -w"; do
pop_repo
done
-test_expect_success 'corrupt tree' '
+test_expect_success 'too-short tree' '
echo abc >malformed-tree &&
- test_must_fail git hash-object -t tree malformed-tree
+ test_must_fail git hash-object -t tree malformed-tree 2>err &&
+ test_i18ngrep "too-short tree object" err
+'
+
+hex2oct() {
+ perl -ne 'printf "\\%03o", hex for /../g'
+}
+
+test_expect_success 'malformed mode in tree' '
+ hex_sha1=$(echo foo | git hash-object --stdin -w) &&
+ bin_sha1=$(echo $hex_sha1 | hex2oct) &&
+ printf "9100644 \0$bin_sha1" >tree-with-malformed-mode &&
+ test_must_fail git hash-object -t tree tree-with-malformed-mode 2>err &&
+ test_i18ngrep "malformed mode in tree entry" err
+'
+
+test_expect_success 'empty filename in tree' '
+ hex_sha1=$(echo foo | git hash-object --stdin -w) &&
+ bin_sha1=$(echo $hex_sha1 | hex2oct) &&
+ printf "100644 \0$bin_sha1" >tree-with-empty-filename &&
+ test_must_fail git hash-object -t tree tree-with-empty-filename 2>err &&
+ test_i18ngrep "empty filename in tree entry" err
'
test_expect_success 'corrupt commit' '
diff --git a/tree-walk.c b/tree-walk.c
index ce27842..24f9a0f 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -27,12 +27,14 @@ static void decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned
const char *path;
unsigned int mode, len;
- if (size < 24 || buf[size - 21])
- die("corrupt tree file");
+ if (size < 23 || buf[size - 21])
+ die(_("too-short tree object"));
path = get_mode(buf, &mode);
- if (!path || !*path)
- die("corrupt tree file");
+ if (!path)
+ die(_("malformed mode in tree entry for tree"));
+ if (!*path)
+ die(_("empty filename in tree entry for tree"));
len = strlen(path) + 1;
/* Initialize the descriptor entry */
@@ -81,7 +83,7 @@ void update_tree_entry(struct tree_desc *desc)
unsigned long len = end - (const unsigned char *)buf;
if (size < len)
- die("corrupt tree file");
+ die(_("too-short tree file"));
buf = end;
size -= len;
desc->buffer = buf;
--
2.8.0.rc4.22.g8ae061a
^ permalink raw reply related
* [PATCH v4 2/2] fsck: handle bad trees like other errors
From: David Turner @ 2016-09-27 20:59 UTC (permalink / raw)
To: git, peff; +Cc: David Turner
In-Reply-To: <1475009991-16368-1-git-send-email-dturner@twosigma.com>
Instead of dying when fsck hits a malformed tree object, log the error
like any other and continue. Now fsck can tell the user which tree is
bad, too.
Signed-off-by: David Turner <dturner@twosigma.com>
---
fsck.c | 18 ++++++++-----
t/t1450-fsck.sh | 16 +++++++++--
tree-walk.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++--------
tree-walk.h | 8 ++++++
4 files changed, 106 insertions(+), 19 deletions(-)
diff --git a/fsck.c b/fsck.c
index c9cf3de..4a3069e 100644
--- a/fsck.c
+++ b/fsck.c
@@ -347,8 +347,9 @@ static int fsck_walk_tree(struct tree *tree, void *data, struct fsck_options *op
return -1;
name = get_object_name(options, &tree->object);
- init_tree_desc(&desc, tree->buffer, tree->size);
- while (tree_entry(&desc, &entry)) {
+ if (init_tree_desc_gently(&desc, tree->buffer, tree->size))
+ return -1;
+ while (tree_entry_gently(&desc, &entry)) {
struct object *obj;
int result;
@@ -520,7 +521,7 @@ static int verify_ordered(unsigned mode1, const char *name1, unsigned mode2, con
static int fsck_tree(struct tree *item, struct fsck_options *options)
{
- int retval;
+ int retval = 0;
int has_null_sha1 = 0;
int has_full_path = 0;
int has_empty_name = 0;
@@ -535,7 +536,10 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)
unsigned o_mode;
const char *o_name;
- init_tree_desc(&desc, item->buffer, item->size);
+ if (init_tree_desc_gently(&desc, item->buffer, item->size)) {
+ retval += report(options, &item->object, FSCK_MSG_BAD_TREE, "cannot be parsed as a tree");
+ return retval;
+ }
o_mode = 0;
o_name = NULL;
@@ -556,7 +560,10 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)
is_hfs_dotgit(name) ||
is_ntfs_dotgit(name));
has_zero_pad |= *(char *)desc.buffer == '0';
- update_tree_entry(&desc);
+ if (update_tree_entry_gently(&desc)) {
+ retval += report(options, &item->object, FSCK_MSG_BAD_TREE, "cannot be parsed as a tree");
+ break;
+ }
switch (mode) {
/*
@@ -597,7 +604,6 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)
o_name = name;
}
- retval = 0;
if (has_null_sha1)
retval += report(options, &item->object, FSCK_MSG_NULL_SHA1, "contains entries pointing to null sha1");
if (has_full_path)
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 8f52da2..ee7d473 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -188,8 +188,7 @@ test_expect_success 'commit with NUL in header' '
grep "error in commit $new.*unterminated header: NUL at offset" out
'
-test_expect_success 'malformatted tree object' '
- test_when_finished "git update-ref -d refs/tags/wrong" &&
+test_expect_success 'tree object with duplicate entries' '
test_when_finished "remove_object \$T" &&
T=$(
GIT_INDEX_FILE=test-index &&
@@ -208,6 +207,19 @@ test_expect_success 'malformatted tree object' '
grep "error in tree .*contains duplicate file entries" out
'
+test_expect_success 'unparseable tree object' '
+ test_when_finished "git update-ref -d refs/heads/wrong" &&
+ test_when_finished "remove_object \$tree_sha1" &&
+ test_when_finished "remove_object \$commit_sha1" &&
+ tree_sha1=$(printf "100644 \0twenty-bytes-of-junk" | git hash-object -t tree --stdin -w --literally) &&
+ commit_sha1=$(git commit-tree $tree_sha1) &&
+ git update-ref refs/heads/wrong $commit_sha1 &&
+ test_must_fail git fsck 2>out &&
+ test_i18ngrep "error: empty filename in tree entry" out &&
+ test_i18ngrep "$tree_sha1" out &&
+ test_i18ngrep ! "fatal: empty filename in tree entry" out
+'
+
test_expect_success 'tag pointing to nonexistent' '
cat >invalid-tag <<-\EOF &&
object ffffffffffffffffffffffffffffffffffffffff
diff --git a/tree-walk.c b/tree-walk.c
index 24f9a0f..828f435 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -22,33 +22,60 @@ static const char *get_mode(const char *str, unsigned int *modep)
return str;
}
-static void decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned long size)
+static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned long size, struct strbuf *err)
{
const char *path;
unsigned int mode, len;
- if (size < 23 || buf[size - 21])
- die(_("too-short tree object"));
+ if (size < 23 || buf[size - 21]) {
+ strbuf_addstr(err, _("too-short tree object"));
+ return -1;
+ }
path = get_mode(buf, &mode);
- if (!path)
- die(_("malformed mode in tree entry for tree"));
- if (!*path)
- die(_("empty filename in tree entry for tree"));
+ if (!path) {
+ strbuf_addstr(err, _("malformed mode in tree entry"));
+ return -1;
+ }
+ if (!*path) {
+ strbuf_addstr(err, _("empty filename in tree entry"));
+ return -1;
+ }
len = strlen(path) + 1;
/* Initialize the descriptor entry */
desc->entry.path = path;
desc->entry.mode = canon_mode(mode);
desc->entry.oid = (const struct object_id *)(path + len);
+
+ return 0;
}
-void init_tree_desc(struct tree_desc *desc, const void *buffer, unsigned long size)
+static int init_tree_desc_internal(struct tree_desc *desc, const void *buffer, unsigned long size, struct strbuf *err)
{
desc->buffer = buffer;
desc->size = size;
if (size)
- decode_tree_entry(desc, buffer, size);
+ return decode_tree_entry(desc, buffer, size, err);
+ return 0;
+}
+
+void init_tree_desc(struct tree_desc *desc, const void *buffer, unsigned long size)
+{
+ struct strbuf err = STRBUF_INIT;
+ if (init_tree_desc_internal(desc, buffer, size, &err))
+ die("%s", err.buf);
+ strbuf_release(&err);
+}
+
+int init_tree_desc_gently(struct tree_desc *desc, const void *buffer, unsigned long size)
+{
+ struct strbuf err = STRBUF_INIT;
+ int result = init_tree_desc_internal(desc, buffer, size, &err);
+ if (result)
+ error("%s", err.buf);
+ strbuf_release(&err);
+ return result;
}
void *fill_tree_descriptor(struct tree_desc *desc, const unsigned char *sha1)
@@ -75,7 +102,7 @@ static void entry_extract(struct tree_desc *t, struct name_entry *a)
*a = t->entry;
}
-void update_tree_entry(struct tree_desc *desc)
+static int update_tree_entry_internal(struct tree_desc *desc, struct strbuf *err)
{
const void *buf = desc->buffer;
const unsigned char *end = desc->entry.oid->hash + 20;
@@ -89,7 +116,30 @@ void update_tree_entry(struct tree_desc *desc)
desc->buffer = buf;
desc->size = size;
if (size)
- decode_tree_entry(desc, buf, size);
+ return decode_tree_entry(desc, buf, size, err);
+ return 0;
+}
+
+void update_tree_entry(struct tree_desc *desc)
+{
+ struct strbuf err = STRBUF_INIT;
+ if (update_tree_entry_internal(desc, &err))
+ die("%s", err.buf);
+ strbuf_release(&err);
+}
+
+int update_tree_entry_gently(struct tree_desc *desc)
+{
+ struct strbuf err = STRBUF_INIT;
+ if (update_tree_entry_internal(desc, &err)) {
+ error("%s", err.buf);
+ strbuf_release(&err);
+ /* Stop processing this tree after error */
+ desc->size = 0;
+ return -1;
+ }
+ strbuf_release(&err);
+ return 0;
}
int tree_entry(struct tree_desc *desc, struct name_entry *entry)
@@ -102,6 +152,17 @@ int tree_entry(struct tree_desc *desc, struct name_entry *entry)
return 1;
}
+int tree_entry_gently(struct tree_desc *desc, struct name_entry *entry)
+{
+ if (!desc->size)
+ return 0;
+
+ *entry = desc->entry;
+ if (update_tree_entry_gently(desc))
+ return 0;
+ return 1;
+}
+
void setup_traverse_info(struct traverse_info *info, const char *base)
{
int pathlen = strlen(base);
diff --git a/tree-walk.h b/tree-walk.h
index 97a7d69..68bb78b 100644
--- a/tree-walk.h
+++ b/tree-walk.h
@@ -25,14 +25,22 @@ static inline int tree_entry_len(const struct name_entry *ne)
return (const char *)ne->oid - ne->path - 1;
}
+/*
+ * The _gently versions of these functions warn and return false on a
+ * corrupt tree entry rather than dying,
+ */
+
void update_tree_entry(struct tree_desc *);
+int update_tree_entry_gently(struct tree_desc *);
void init_tree_desc(struct tree_desc *desc, const void *buf, unsigned long size);
+int init_tree_desc_gently(struct tree_desc *desc, const void *buf, unsigned long size);
/*
* Helper function that does both tree_entry_extract() and update_tree_entry()
* and returns true for success
*/
int tree_entry(struct tree_desc *, struct name_entry *);
+int tree_entry_gently(struct tree_desc *, struct name_entry *);
void *fill_tree_descriptor(struct tree_desc *desc, const unsigned char *sha1);
--
2.8.0.rc4.22.g8ae061a
^ permalink raw reply related
* Re: [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, part 2
From: René Scharfe @ 2016-09-27 20:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
In-Reply-To: <xmqqshslm6jo.fsf@gitster.mtv.corp.google.com>
Am 27.09.2016 um 22:28 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>> diff --git a/submodule.c b/submodule.c
>> index dcc5ce3..8cf40ea 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -396,7 +396,7 @@ static void show_submodule_header(FILE *f, const char *path,
>> find_unique_abbrev(one->hash, DEFAULT_ABBREV));
>> if (!fast_backward && !fast_forward)
>> strbuf_addch(&sb, '.');
>> - strbuf_addstr(&sb, find_unique_abbrev(two->hash, DEFAULT_ABBREV));
>> + strbuf_add_unique_abbrev(&sb->hash, two, DEFAULT_ABBREV);
>
> I wonder how could this change come out of this definition:
>
> @@
> expression E1, E2, E3;
> @@
> - strbuf_addstr(E1, find_unique_abbrev(E2, E3));
> + strbuf_add_unique_abbrev(E1, E2, E3);
Impossible. I added "->hash" manually during a rebase (merging
a0d12c44, wrongly). Good catch, thanks!
Seeing proof of skipping compile-testing I wonder what else I do
forget in my daily life. :-| I'll better go to sleep now..
Fixup patch, generated by reverting the diff, re-adding the
semantic patch and using coccicheck; compiles and survives make
test:
---
submodule.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/submodule.c b/submodule.c
index 8cf40ea..bb06b60 100644
--- a/submodule.c
+++ b/submodule.c
@@ -396,7 +396,7 @@ static void show_submodule_header(FILE *f, const char *path,
find_unique_abbrev(one->hash, DEFAULT_ABBREV));
if (!fast_backward && !fast_forward)
strbuf_addch(&sb, '.');
- strbuf_add_unique_abbrev(&sb->hash, two, DEFAULT_ABBREV);
+ strbuf_add_unique_abbrev(&sb, two->hash, DEFAULT_ABBREV);
if (message)
strbuf_addf(&sb, " %s%s\n", message, reset);
else
--
2.10.0
^ permalink raw reply related
* Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules
From: Stefan Beller @ 2016-09-27 20:59 UTC (permalink / raw)
To: Brandon Williams; +Cc: Junio C Hamano, git@vger.kernel.org
In-Reply-To: <20160927205202.GG32565@google.com>
On Tue, Sep 27, 2016 at 1:52 PM, Brandon Williams <bmwill@google.com> wrote:
> On 09/27, Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> > In nul_to_q and q_to_nul implementations (t/test-lib-functions.sh)
>> > we seem to avoid using "tr", even though q_to_cr and others do use
>> > it. I wonder if we had some portability issues with passing NUL
>> > through tr or something?
>> >
>> > ... digs and finds e85fe4d8 ("more tr portability test script
>> > fixes", 2008-03-12)
>> >
>> > So use something like
>> >
>> > perl -pe 'y/\012/\000/' <<\-EOF
>> > ...
>> > EOF
>> >
>> > instead, perhaps?
>>
>> I actually think it would make more sense to add
>>
>> lf_to_nul () {
>> perl -pe 'y/\012/\000/'
>> }
>>
>> to t/test-lib-functions.sh somewhere near q_to_nul if we were to go
>> this route.
>
> my mind is drawing a blank, what does the 'lf' in 'lf_to_nul' stand for?
> line feed?
>
Yes, line feed. (Note that Git has to deal with this cross platform new lines
e.g. CRLF is common on Windows, CR was common on MAC, and LF is
Windows, so naming the new line as they are, makes sense here.)
^ permalink raw reply
* Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules
From: Junio C Hamano @ 2016-09-27 20:58 UTC (permalink / raw)
To: Brandon Williams; +Cc: git
In-Reply-To: <20160927205202.GG32565@google.com>
Brandon Williams <bmwill@google.com> writes:
> my mind is drawing a blank, what does the 'lf' in 'lf_to_nul' stand for?
> line feed?
Yup. "man 7 ascii" ;-)
^ permalink raw reply
* RE: git-upload-pack hangs
From: Jason Pyeron @ 2016-09-27 20:56 UTC (permalink / raw)
To: git; +Cc: 'Junio C Hamano'
In-Reply-To: <xmqqlgydqqk0.fsf@gitster.mtv.corp.google.com>
> -----Original Message-----
> From: Junio C Hamano
> Sent: Tuesday, September 27, 2016 12:02
>
> Jason Pyeron writes:
>
> > This is a very, very first draft.
> >
> > It is allowing IIS to work right now.
> >
> > I still need to address chunked issues, where there is no
> content length (see
> http://www.gossamer-threads.com/lists/apache/users/373042)
> >
> > Any comments, sugestions?
>
> Does this have any relation to another thread earlier this year,
> which seems to have ended here:
>
> https://public-inbox.org/git/20160401235532.GA27941@sigill.intra.peff.net/
Looks like the same. Did not (don't know why I was unsubscribed a few months ago?!) see that one and gmane is killing me on my
searches...
I will read the full thread, digest and fold what I can glean in to my (in production right now) patch.
-Jason
^ permalink raw reply
* Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules
From: Brandon Williams @ 2016-09-27 20:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqwphxm7av.fsf@gitster.mtv.corp.google.com>
On 09/27, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > In nul_to_q and q_to_nul implementations (t/test-lib-functions.sh)
> > we seem to avoid using "tr", even though q_to_cr and others do use
> > it. I wonder if we had some portability issues with passing NUL
> > through tr or something?
> >
> > ... digs and finds e85fe4d8 ("more tr portability test script
> > fixes", 2008-03-12)
> >
> > So use something like
> >
> > perl -pe 'y/\012/\000/' <<\-EOF
> > ...
> > EOF
> >
> > instead, perhaps?
>
> I actually think it would make more sense to add
>
> lf_to_nul () {
> perl -pe 'y/\012/\000/'
> }
>
> to t/test-lib-functions.sh somewhere near q_to_nul if we were to go
> this route.
my mind is drawing a blank, what does the 'lf' in 'lf_to_nul' stand for?
line feed?
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules
From: Brandon Williams @ 2016-09-27 20:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqh991nq34.fsf@gitster.mtv.corp.google.com>
On 09/27, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> > @@ -170,6 +171,27 @@ static void show_killed_files(struct dir_struct *dir)
> > }
> > }
> >
> > +/*
> > + * Compile an argv_array with all of the options supported by --recurse_submodules
> > + */
> > +static void compile_submodule_options(int show_tag)
> > +{
> > + if (show_cached)
> > + argv_array_push(&recurse_submodules_opts, "--cached");
> > + if (show_stage)
> > + argv_array_push(&recurse_submodules_opts, "--stage");
> > + if (show_valid_bit)
> > + argv_array_push(&recurse_submodules_opts, "-v");
> > + if (show_tag)
> > + argv_array_push(&recurse_submodules_opts, "-t");
> > + if (line_terminator == '\0')
> > + argv_array_push(&recurse_submodules_opts, "-z");
> > + if (debug_mode)
> > + argv_array_push(&recurse_submodules_opts, "--debug");
> > + if (show_eol)
> > + argv_array_push(&recurse_submodules_opts, "--eol");
> > +}
>
> OK. These are only the safe ones to pass through? "compile" or
> "assemble" is much less important thing to say than how these are
> chosen. "pass_supported_options()" or something? I dunno.
Alternatively we can change this to compile all potential options (not
just the ones that are supported now) and then just error the caller
out, as you've suggested, if an unsupported option used. 'pass' may not
be the most descriptive word for this function as it isn't actually
doing the passing but rather generating an argv of options that will be
passed in the event a submodule is found and we need to kick off a child
process for it.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules
From: Brandon Williams @ 2016-09-27 20:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqd1jpnpyh.fsf@gitster.mtv.corp.google.com>
On 09/27, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
>
> > if (recurse_submodules &&
> > - (show_stage || show_deleted || show_others || show_unmerged ||
> > + (show_deleted || show_others || show_unmerged ||
> > show_killed || show_modified || show_resolve_undo ||
> > - show_valid_bit || show_tag || show_eol || with_tree ||
> > - (line_terminator == '\0')))
> > + with_tree))
> > die("ls-files --recurse-submodules unsupported mode");
>
> Ahh, one more thing, and this comment probably applies to 2/4 not
> this one, but if the intention is to shrink this "not supported yet"
> check as the series progresses, in the earlier step the check would
> need to make sure no pathspec is given, which is first supported in
> 4/4, I think. It is not a big deal to require rerolling by itself,
> bit if you are rerolling 2/4 for other reasons, then...
Oh there is a separate if gaurd for pathspecs which is introduced in 2/4
and then removed once pathspec support has been added in 4/4.
--
Brandon Williams
^ permalink raw reply
* [PATCH v2] rev-list-options: clarify the usage of --reverse
From: Pranit Bauva @ 2016-09-27 20:44 UTC (permalink / raw)
To: git
In-Reply-To: <010201576bfb6c7d-0b68228f-9503-4dd1-9721-713477fa2596-000000@eu-west-1.amazonses.com>
Specify even more clearly that --reverse works only with the commits
which are chosen to be shown so as to eliminate the confusion as to
whether the first n or the last n commits with be shown when used
with `-n --reverse`.
Reported-by: Ruediger Meier <sweet_f_a@gmx.de>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
Documentation/rev-list-options.txt | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 7e462d3..5da7cf5 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -657,8 +657,9 @@ avoid showing the commits from two parallel development track mixed
together.
--reverse::
- Output the commits in reverse order.
- Cannot be combined with `--walk-reflogs`.
+ Output the commits chosen to be shown (see Commit Limiting
+ section above) in reverse order. Cannot be combined with
+ `--walk-reflogs`.
Object Traversal
~~~~~~~~~~~~~~~~
--
https://github.com/git/git/pull/296
^ permalink raw reply related
* Re: [PATCH 1/4 v4] submodules: make submodule-prefix option
From: Brandon Williams @ 2016-09-27 20:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqoa39m67l.fsf@gitster.mtv.corp.google.com>
On 09/27, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
>
> >> s/submodules/submodule-prefix/ at least.
> >
> > So should the #define be SUPPORT_SUBMODULE_PREFIX instead? That may be
> > too narrow minded and not looking toward future submodule options
> > support but I'm not sure.
>
> I am not convinced that this prefix needs to be tied/limited to
> submodule, at least not yet, though. I view it as a prefix that
> points from above the repository's top, of which submodule support
> may be a good sample user, but the caller may not necessarily be
> doing or interested in "submodule support".
>
> That's also part of figuring out how we want define the semantics of
> this thing and how we want to present it to the end-users, I guess,
> so we may have to rename it when we know more, but that's OK.
K we can leave the name as is for now then. Thankfully it should be a
simple name change since it only lives in 1 file.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 4/4 v4] ls-files: add pathspec matching for submodules
From: Brandon Williams @ 2016-09-27 20:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq1t05nmcs.fsf@gitster.mtv.corp.google.com>
On 09/27, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
>
> > - /* 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;
>
> I still wonder if we can do better than this, as this would be a big
> cycle-saver especially in recurse-submodules case.
>
> When you get max_prefix that is "a/b/c", there are three cases:
>
> * a/b/c is a path prefix for an entry in the index, e.g. a/b/c/d;
> you then can safely use it and you do not have to do any
> recursive invocation of ls-files outside "a/b/c". You may match
> a/b/c/d in the toplevel, or you may recurse a/b/c/e that is a
> submodule, but you won't have to pay attention to submodules
> outside.
>
> * a leading path of a/b/c, e.g. a/b, is a gitlink or a blob in the
> index; you can use a/b and you only have to recurse into a/b if
> that is a submodule; if a/b is a blob, you'd show nothing.
>
> * a/b/c itself and no leading path of it appears in the index; you
> know that nothing will match once you know that you are in this
> situation.
>
> Because a gitlink "a/b" sorts at the same location in the index as a
> regular blob "a/b" would, by feeding the max_prefix common_prefix()
> gives you (i.e. "a/b/c") to index_name_pos() to see which one of the
> three situations you are in can be done fairly cheaply, I would
> think. The index_name_pos() call may find "a/b/c" exactly (case 1),
> or return a location where "a/b/c" would be inserted in the list of
> existing entries. If there were "a/b" (or "a") in the index, there
> wouldn't be any "a/b/x" (or "a/x") at the same time, so a query for
> "a/b/c" would land you next to (just after) an existing entry that
> is a leading path of it, if such an entry exists, no? That would
> allow you to tell case 2 above fairly cheaply, I would expect.
>
> It is a separate issue if adding that support to 4/4 is a good idea;
> I personally think doing it as a separate follow-up patch would make
> more sense, so all of the above is tangent.
I agree that this could be a big cycle saver. At the present I was
working towards getting a working implementation but this should
definitely be addressed in a follow-up patch to introduce the
optimization to the recurse-submodule mode. It hopefully wouldn't be
too hard to implement seeing as its using string literals.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 1/4 v4] submodules: make submodule-prefix option
From: Junio C Hamano @ 2016-09-27 20:35 UTC (permalink / raw)
To: Brandon Williams; +Cc: git
In-Reply-To: <20160927202925.GA32565@google.com>
Brandon Williams <bmwill@google.com> writes:
>> s/submodules/submodule-prefix/ at least.
>
> So should the #define be SUPPORT_SUBMODULE_PREFIX instead? That may be
> too narrow minded and not looking toward future submodule options
> support but I'm not sure.
I am not convinced that this prefix needs to be tied/limited to
submodule, at least not yet, though. I view it as a prefix that
points from above the repository's top, of which submodule support
may be a good sample user, but the caller may not necessarily be
doing or interested in "submodule support".
That's also part of figuring out how we want define the semantics of
this thing and how we want to present it to the end-users, I guess,
so we may have to rename it when we know more, but that's OK.
^ 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