Re: Working with zip files

[Jacob Keller]

On Tue, Aug 16, 2016 at 2:14 PM, Nikolaus Rath  wrote:
> On Aug 16 2016, David Lang  wrote:
>> On Tue, 16 Aug 2016, Nikolaus Rath wrote:
>>
>>> I would like to store Simulink models in a Git
>>> repository. Unfortunately, the file format is binary. But luckily, the
>>> binary format happens to be a zipfile containing nicely formatted XML
>>> files.
>>>
>>> Is there a way to teach Git to take advantage of this when storing,
>>> diff-ing and merging these files?
>>
>> you should be able to use clean/smudge to have git store the files
>> uncompressed, which will help a lot.
>
> Having looked at that, I'm not sure if this really helps:
>
> As I understand, the smudge command is run on checkout to convert the
> blob in the repository to the format that is desired in the working
> tree. But this is the opposite of what I need: on checkout, I need to
> convert the text data in the repository to a blob in the working tree.
>
> Furthermore, I need to convert multiple text files into one blob, will
> smudge/clean seem to do just 1:1 conversions.
>
> Am I missing something? Are there any other options?

You want to store the contents of the zip file as *one* blob that is
the uncompressed contents of the archive somehow concatenated
together. That should still be a 1:1 relationship.

You won't store one blob per file in the zip.

Thanks,
Jake
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git-mergetool reverse file ordering

[David Aguilar]

On Mon, Aug 15, 2016 at 09:19:35PM +0100, Luis Gutierrez wrote:
> > Thoughts?  Would you be interested in helping work up a patch
> > for this idea?  At a minimum we should also write a test case in
> > t/t7610-mergetool.sh to verify that it works as advertised.
> 
> > Why not reuse the existing diff.orderFile config variable?  (Also
> > supported by the -O option to git-diff).
> 
> 
> I'll be happy to write a testcase, and to re-use the -O
> (diff.orderFile config var) option to git-diff as sugguested by John
> Keeping.
> 
> Is this the final spec?
> 
> 
> 
> I'll be happy to do that.
> 
> Luis

Hmm, I do like the idea of reusing the diff orderFile, but a
mechanism for sorting arbitrary inputs based on the orderFile
isn't currently exposed in a way that mergetool could use it.


Looking at the code in mergetool, we basically need something
that has the same spec as "sort" itself, namely that it can take
arbitrary arbitrary input on stdin and sort it.


Implementing the orderFile support would probably be best done
in C.  Would we want to expose it as an internal helper?

e.g.
git diff--order-helper 

could be used to perform the sorting.

But, that sort is honestly kinda crude.  It can't implement the
interesting case where we want bar.h to sort before bar.c but
not foo.h.

If we did the sort option, we could have both.

Thoughts?

-- 
David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Draft of Git Rev News edition 18

[Josh Triplett]

On Tue, Aug 16, 2016 at 09:27:04PM +, Eric Wong wrote:
> Josh Triplett  wrote:
> > On Tue, Aug 16, 2016 at 09:30:27AM +, Eric Wong wrote:
> > > Jakub Narębski  wrote:
> > > > It's a great pity that https://public-inbox.org/ is just
> > > > directory index, not a true home page.
> > > 
> > > +Cc m...@public-inbox.org
> > > 
> > > I'm not sure one could do better while staying true to the
> > > minimalist nature of plain-text email.
> > > 
> > > In the spirit of decentralization, there may not be /a/
> > > homepage, but many.   Everything is meant to clonable with each
> > > public-inbox, so maybe every public-inbox will have a code
> > > branch attached to it with the source+docs bundled.
> > 
> > It'd be nice if it had a prominent list of all lists available; as far
> > as I can tell, the main page has no link to /git/.
> 
> I'm not sure that's necessary; most of the traffic seems to come
> from /git/MESSAGE_ID/ links posted by others.  So it's
> probably more inside-out exposure than anything.

If someone hears about public-inbox, it's nice to know what other lists
they can use it for.

> As for other projects, I'm not aware of anybody else using it,
> yet.  I have some small projects using it, but most of those are
> one-off throwaways and I'm not comfortable promoting those along
> with public-inbox.  I admit: I'm not comfortable promoting
> anything I do, really.

Please take this as encouragement to do so.  I'd love to see the
public-inbox equivalent to the main page of https://lists.debian.org/ ,
as an example.  (And I'd love to have public-inbox archives of Debian
mailing lists.)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: upload-pack/ls-remote: showing non-HEAD symbolic refs?

[Josh Triplett]

On Tue, Aug 16, 2016 at 05:15:51PM -0400, Jeff King wrote:
> On Tue, Aug 16, 2016 at 02:11:41PM -0700, Josh Triplett wrote:
> 
> > > For HTTPS, I'd just as soon use HTTP-level features.
> > 
> > ALPN, used carefully, could potentially allow eliminating one round-trip
> > compared to HTTPS, and could also allow full-duplex communication.
> 
> I'd love to have a real full-duplex git-over-https. I looked into
> WebSockets at one point, but it looked non-trivial and I gave up.

WebSockets would be non-trivial, and require server configuration as
well, but it could work.

> But if we had a real full-duplex connection over https, I think there
> would be no reason for git:// to continue existing (we'd probably keep
> ssh as it's a useful protocol for authentication, though).

Agreed.

Using ALPN wouldn't actually end up using HTTPS; it would negotiate with
the server and end up connected directly to a git program speaking an
arbitrary protocol over TLS.  Many web servers already support ALPN to
negotiate HTTP/2, so this seems plausible.

Another alternative would be to define a framing for a full-duplex
git-upload-pack connection inside a single HTTP/2 connection; HTTP/2
already effectively allows full-duplex asynchronous conversations.

- Josh Triplett
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Prefer "long" key format output when verifying pgp signatures

[Junio C Hamano]

Linus Torvalds  writes:

> From: Linus Torvalds 
> Date: Tue, 16 Aug 2016 13:10:24 -0700
> Subject: [PATCH] Prefer "long" key format output when verifying pgp signatures
>
> Yes, gpg2 already uses the long format by default, but most
> distributions seem to still have "gpg" be the older 1.x version due to
> compatibility reasons.  And older versions of gpg only show the 32-bit
> short ID, which is quite insecure.
> ...
> But the 32-bit key ID's really are broken. Also note that because of the 
> differences between gpg-1.x and gpg-2.x, hopefully any scripted key ID 
> parsing code (if such code exists) is already flexible enough to not care.
>
> This was triggered by the fact that the "evil32" project keys ended up
> leaking to the public key servers, so now there are 32-bit aliases for
> just about every open source developer that you can easily get by
> mistake if you use the 32-bit short ID format.
>
> Signed-off-by: Linus Torvalds 
> ---
>
> That's a very long commit message for a very trivial patch.
>
> I'm not particularly happy with the 64-bit long format either, but it's 
> better than what we have now, and appears to be as good as it gets.

Thanks.  Will queue.

>
>  gpg-interface.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/gpg-interface.c b/gpg-interface.c
> index 08356f92e7b3..8672edaf4823 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -217,6 +217,7 @@ int verify_signed_buffer(const char *payload, size_t 
> payload_size,
>   argv_array_pushl(,
>gpg_program,
>"--status-fd=1",
> +  "--keyid-format=long",
>"--verify", temp.filename.buf, "-",
>NULL);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 1/3] doc: commit: --fixup/--squash can take a commit revision

[Philip Oakley]

From: "Philip Oakley" 

From: "Junio C Hamano" 

"Philip Oakley"  writes:


I think the
use of "commit" in an angle-bracket-pair in the label for the
section, i.e. "--fixup=", has been considered to be clear
enough to tell that you can use usual extended SHA-1 syntax to
specify the commit you want to talk about,


I certainly hadn't picked up on that ability to use the extended sha1
syntax (specifying revisions...) here.


By "has been considered", I meant that the documentation text is
still open for improvement.  I just didn't find rewording "commit"
with "commit revision" is that improvement we need there.

Perhaps we need to have somewhere central a section that explains
various notations used in the documentation set.  I think it is safe
to say something like "unless otherwise qualified,  (or any
object type in an angle-bracket-pair) is used as a placeholder to
take any acceptable way to spell object names (cf. gitrevisions for
details)" these days [*1*].


True. I'm cautious that we may accidentally still hide it in another 
document that the user doesn't see when reading "this" (or any other)  man 
page.


Your sentence is short enough to be added to those few key pages that 
users refer to to get them started in the right direction.



*1* In ancient days I think some plumbing commands only took 40-hex
object names, but as far as I know they've all been updated.
--


I'll take the patch series away and have a rework over the coming week or 
so.


Philip 


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 3/3] diff: add SUBMODULE_DIFF format to display submodule diff

[Junio C Hamano]

Jacob Keller  writes:

> Thoughts on that? Or should we just limit ourselves to only some
> options get propagated to the submodule?

I think you have to be selective either way.  You do not want
pathspecs used to limit the top-level paths propagated down when you
run a diff or a log in the submodule, for example, and that does not
change even if you start using the code in diff-lib.c (after adding
the submodule odb as an alternate).

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: storing cover letter of a patch series?

[Philip Oakley]

From: "Duy Nguyen" 
On Tue, Aug 16, 2016 at 12:26 PM, Jacob Keller  
wrote:
They can just add "squash! cover! " commits for that ;-) Though 
more
likely the advanced workflow would be used... We'll need both (more 
than

one) options.


Or even better, "git commit --reword $SHA1" brings up the editor with
commit message of $SHA1. Modify any way you want and it creates a new
empty, "reword!" commit that contains the diff between the old commit
message and the new one. "reword!" can be consumed by "rebase -i
--autosquash" without bringing up the editor again. I realize making
"git commit --reword" run multiple times would be tricky though...


I was just thinking you write text and it gets appended to the text of
the reworded commit, and when you squash them using rebase you get to
finalize it like a normal squash?


I think that's what Phillip meant by 'squash! cover!' though I wanted
to go further, I don't want an editor popping up at rebase time,
instead 'rebase' just update cover letter automatically for me.
--

Hi Duy,
While we can have code that is auto merged, I don't think that I'd want to 
submit a cover letter that was simply auto merged. I'd want to refresh and 
re-personalise the text. As long as the flexibility in our cover letter 
inclusion is there

--
Philip

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Draft of Git Rev News edition 18

[Eric Wong]

Josh Triplett  wrote:
> On Tue, Aug 16, 2016 at 09:30:27AM +, Eric Wong wrote:
> > Jakub Narębski  wrote:
> > > It's a great pity that https://public-inbox.org/ is just
> > > directory index, not a true home page.
> > 
> > +Cc m...@public-inbox.org
> > 
> > I'm not sure one could do better while staying true to the
> > minimalist nature of plain-text email.
> > 
> > In the spirit of decentralization, there may not be /a/
> > homepage, but many.   Everything is meant to clonable with each
> > public-inbox, so maybe every public-inbox will have a code
> > branch attached to it with the source+docs bundled.
> 
> It'd be nice if it had a prominent list of all lists available; as far
> as I can tell, the main page has no link to /git/.

I'm not sure that's necessary; most of the traffic seems to come
from /git/MESSAGE_ID/ links posted by others.  So it's
probably more inside-out exposure than anything.

As for other projects, I'm not aware of anybody else using it,
yet.  I have some small projects using it, but most of those are
one-off throwaways and I'm not comfortable promoting those along
with public-inbox.  I admit: I'm not comfortable promoting
anything I do, really.

I do wish more people would start using the .onions, though...
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 3/3] diff: add SUBMODULE_DIFF format to display submodule diff

[Jacob Keller]

On Tue, Aug 16, 2016 at 2:14 PM, Junio C Hamano  wrote:
> Jacob Keller  writes:
>
 +
 + if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED) {
 + /*
 +  * If the submodule has modified contents we want to diff
 +  * against the work tree, so don't add a second parameter.
 +  * This is essentially equivalent of using diff-index 
 instead.
 +  * Note that we can't (easily) show the diff of any untracked
 +  * files.
 +  */
>>> ...
>>> I am debating myself if this is a good thing to do, though.  The
>>> submodule is a separate project for a reason, and there is a reason
>>> why the changes haven't been committed.  When asking "what's different
>>> between these two superproject states?", should the user really see
>>> these uncommitted changes?
>>
>> Well, the previous submodule code for "log" prints out "submodule has
>> untracked content" and "submodule has modified content" so I figured
>> the diff might want to show that as a diff too? Or maybe we just stick
>> with the messages and only show the difference of what's actually been
>> committed.. I think that's probably ok too.
>
> I do not have a strong opinion.  We only see DIRTY when we are
> looking at the working tree at the top-level diff (i.e. "git diff
> HEAD~ HEAD" at the top-level, or "git diff --cached" will not show
> DIRTY_SUBMODULE_MODIFIED when a submodule has local modifications in
> its working tree), so in that sense, it probably makes sense to show
> the more detailed picture of the working tree like your patch does.
> After all, choosing to use --submodule=diff is a strong sign that
> the user who says top-level "git diff []" wants to see the
> details of her work-in-progress.
>
> Do you need to handle "git diff -R []" at the top-level a bit
> differently, by the way?  If this function gets the full diff_options
> that is driving the top-level diff, the DIFF_OPT_REVERSE_DIFF bit
> would tell you that.
>

Probably. Unfortunately what I really need is to be able to convert
(almost) all diff options from the diff_options structure back into
command line flags. This is the primary reason I would prefer to not
use a sub-command, but I'm not really sure what's the best way to
actually DO that in a safe way.

The sub command brings nice separation between the superproject and
its submodules... but it has problem because we can't use C calls
directly, so I can't pass the options along myself.

Thoughts on that? Or should we just limit ourselves to only some
options get propagated to the submodule?

Thanks,
Jake
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: upload-pack/ls-remote: showing non-HEAD symbolic refs?

[Jeff King]

On Tue, Aug 16, 2016 at 02:11:41PM -0700, Josh Triplett wrote:

> > For HTTPS, I'd just as soon use HTTP-level features.
> 
> ALPN, used carefully, could potentially allow eliminating one round-trip
> compared to HTTPS, and could also allow full-duplex communication.

I'd love to have a real full-duplex git-over-https. I looked into
WebSockets at one point, but it looked non-trivial and I gave up.

But if we had a real full-duplex connection over https, I think there
would be no reason for git:// to continue existing (we'd probably keep
ssh as it's a useful protocol for authentication, though).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Working with zip files

[Nikolaus Rath]

On Aug 16 2016, David Lang  wrote:
> On Tue, 16 Aug 2016, Nikolaus Rath wrote:
>
>> I would like to store Simulink models in a Git
>> repository. Unfortunately, the file format is binary. But luckily, the
>> binary format happens to be a zipfile containing nicely formatted XML
>> files.
>>
>> Is there a way to teach Git to take advantage of this when storing,
>> diff-ing and merging these files?
>
> you should be able to use clean/smudge to have git store the files
> uncompressed, which will help a lot.

Having looked at that, I'm not sure if this really helps:

As I understand, the smudge command is run on checkout to convert the
blob in the repository to the format that is desired in the working
tree. But this is the opposite of what I need: on checkout, I need to
convert the text data in the repository to a blob in the working tree.

Furthermore, I need to convert multiple text files into one blob, will
smudge/clean seem to do just 1:1 conversions.

Am I missing something? Are there any other options?

Best,
Nikolaus
-- 
GPG encrypted emails preferred. Key id: 0xD113FCAC3C4E599F
Fingerprint: ED31 791B 2C5C 1613 AF38 8B8A D113 FCAC 3C4E 599F

 »Time flies like an arrow, fruit flies like a Banana.«
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 3/3] diff: add SUBMODULE_DIFF format to display submodule diff

[Junio C Hamano]

Jacob Keller  writes:

>>> +
>>> + if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED) {
>>> + /*
>>> +  * If the submodule has modified contents we want to diff
>>> +  * against the work tree, so don't add a second parameter.
>>> +  * This is essentially equivalent of using diff-index instead.
>>> +  * Note that we can't (easily) show the diff of any untracked
>>> +  * files.
>>> +  */
>> ...
>> I am debating myself if this is a good thing to do, though.  The
>> submodule is a separate project for a reason, and there is a reason
>> why the changes haven't been committed.  When asking "what's different
>> between these two superproject states?", should the user really see
>> these uncommitted changes?
>
> Well, the previous submodule code for "log" prints out "submodule has
> untracked content" and "submodule has modified content" so I figured
> the diff might want to show that as a diff too? Or maybe we just stick
> with the messages and only show the difference of what's actually been
> committed.. I think that's probably ok too.

I do not have a strong opinion.  We only see DIRTY when we are
looking at the working tree at the top-level diff (i.e. "git diff
HEAD~ HEAD" at the top-level, or "git diff --cached" will not show
DIRTY_SUBMODULE_MODIFIED when a submodule has local modifications in
its working tree), so in that sense, it probably makes sense to show
the more detailed picture of the working tree like your patch does.
After all, choosing to use --submodule=diff is a strong sign that
the user who says top-level "git diff []" wants to see the
details of her work-in-progress.

Do you need to handle "git diff -R []" at the top-level a bit
differently, by the way?  If this function gets the full diff_options
that is driving the top-level diff, the DIFF_OPT_REVERSE_DIFF bit
would tell you that.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: upload-pack/ls-remote: showing non-HEAD symbolic refs?

[Josh Triplett]

On Tue, Aug 16, 2016 at 04:54:27PM -0400, Jeff King wrote:
> On Tue, Aug 16, 2016 at 01:31:50PM -0700, Josh Triplett wrote:
> 
> > > You can dig up the discussion on the list under the name "protocol v2",
> > > but basically yes, that approach has been considered. It's a little
> > > gross just because it leaves other protocols behind http (and it is not
> > > necessarily a good idea to push people into http, because it has some
> > > fundamental drawbacks over the other protocols because of its
> > > half-duplex nature).
> > 
> > I looked through the "protocol v2" threads, but couldn't find any
> > discussions of using HTTP headers.  I found some mentions of using
> > additional query parameters on the git-upload-pack request, which would
> > break compatibility with existing servers (they won't just ignore the
> > extra parameter).
> 
> Probably the most interesting recent discussion is the sub-thread of
> this patch:
> 
>   
> http://public-inbox.org/git/1460747949-3514-5-git-send-email-dtur...@twopensource.com/
> 
> which you might have missed because it only messages "v2 protocol" in
> the body.

Thanks for the link.

> But basically, I think you get the gist of it. We need to pass
> information from the client to the server _before_ the initial
> capability advertisement. For HTTP, we can do it via specialized headers
> or query parameters. For other protocols, we're stuck with some kind of
> try-and-fallback hack.
> 
> That means those protocols may diverge slightly from HTTP, but at least
> they would differ only in the "bootstrap v2" bit (and that would
> eventually become irrelevant as everybody moves to v2).
> 
> > --client-caps could work for SSH as well, it just requires an extra
> > round-trip to check for --client-caps.  Call git-upload-pack
> > --supports-client-caps, ignore any output (which with current git will
> > consist of a usage message), see if it returns a 0 exit code, if so,
> > call git-upload-pack --client-caps='...', and if not just call
> > git-upload-pack.  (A new git-upload-pack-2 binary would also work, but
> > that seems like overkill.)  I don't see any way around the extra round
> > trip here that would preserve backward compatibility with existing SSH
> > servers (which may force clients to *only* run exactly the command
> > "git-upload-pack" and nothing else).
> 
> Yep, that's about it. For ssh, I suspect we could optimistically try:
> 
>   git upload-pack --v2; test $? = 129 && git-upload-pack
> 
> and then fallback to just "git-upload-pack". That would work without an
> extra round-trip on real shell-capable servers, and eventually work on
> restricted ones.

True, that seems completely sensible.

> That doesn't help git://, though.

I'd love to see plaintext git:// disappear through lack of use.

> There are proposals floating around for basically easing into it with
> config. Have a "remote.*.v2" option you can set locally to enable (or
> disable) it. Default to "false". When there are enough v2 servers around
> to make it worthwhile, flip the default to "auto" which will do the
> probing (at some minor expense of handling fallbacks). Optionally we
> could record the last response for "auto" and use that going forward.

That sounds sensible.

> > Another possibility, which would work for both HTTPS and
> > git-protocol-over-TLS, would be to use ALPN.
> 
> Do people actually use git-over-TLS? There's no core support AFAIK, so
> you'd have to hack it up with a client proxy and git-remote-ext.

I've seen the idea bounced around and prototyped in the context of
supporting authenticated push to git://

> For HTTPS, I'd just as soon use HTTP-level features.

ALPN, used carefully, could potentially allow eliminating one round-trip
compared to HTTPS, and could also allow full-duplex communication.

- Josh Triplett
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] t/Makefile: make sure that file names are truly platform-independent

[Johannes Sixt]

Am 16.08.2016 um 10:42 schrieb Johannes Schindelin:

That only leaves the conclusion that some of our pathspec
code tries to be helpful and takes a backslash for a directory separator.


Very true. The code that does this is in prefix_filename():

https://github.com/git/git/blob/master/abspath.c#L170

-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]

[Eric Wong]

Eric Wong  wrote:
> Currently for web users, I suggest:
> 
>   curl $URL >tmpXXX
> 
>   # open tmp and tag+copy to patchesXXX using MUA of choice:
>   # (also seems to be what Jeff describes):
>   mutt -f tmpXXX
> 
>   git am patches

I should add this is also a better match to an "offline first"
workflow for disconnected use.  My Internet connections drop
all the time :<

I don't mind people over-downloading at all, and stuffing an
extra cover + comments is likely more efficient with gzip than
going back online to fetch separately.

> Perhaps adding checkbox next to each item might work as a
> select-to-include-in-mbox download form.

...So I'm not sure if I want to invest time into this idea
since either the server or user could be offline by the
time messages are selected.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: upload-pack/ls-remote: showing non-HEAD symbolic refs?

[Jeff King]

On Tue, Aug 16, 2016 at 01:31:50PM -0700, Josh Triplett wrote:

> > You can dig up the discussion on the list under the name "protocol v2",
> > but basically yes, that approach has been considered. It's a little
> > gross just because it leaves other protocols behind http (and it is not
> > necessarily a good idea to push people into http, because it has some
> > fundamental drawbacks over the other protocols because of its
> > half-duplex nature).
> 
> I looked through the "protocol v2" threads, but couldn't find any
> discussions of using HTTP headers.  I found some mentions of using
> additional query parameters on the git-upload-pack request, which would
> break compatibility with existing servers (they won't just ignore the
> extra parameter).

Probably the most interesting recent discussion is the sub-thread of
this patch:

  
http://public-inbox.org/git/1460747949-3514-5-git-send-email-dtur...@twopensource.com/

which you might have missed because it only messages "v2 protocol" in
the body.

But basically, I think you get the gist of it. We need to pass
information from the client to the server _before_ the initial
capability advertisement. For HTTP, we can do it via specialized headers
or query parameters. For other protocols, we're stuck with some kind of
try-and-fallback hack.

That means those protocols may diverge slightly from HTTP, but at least
they would differ only in the "bootstrap v2" bit (and that would
eventually become irrelevant as everybody moves to v2).

> --client-caps could work for SSH as well, it just requires an extra
> round-trip to check for --client-caps.  Call git-upload-pack
> --supports-client-caps, ignore any output (which with current git will
> consist of a usage message), see if it returns a 0 exit code, if so,
> call git-upload-pack --client-caps='...', and if not just call
> git-upload-pack.  (A new git-upload-pack-2 binary would also work, but
> that seems like overkill.)  I don't see any way around the extra round
> trip here that would preserve backward compatibility with existing SSH
> servers (which may force clients to *only* run exactly the command
> "git-upload-pack" and nothing else).

Yep, that's about it. For ssh, I suspect we could optimistically try:

  git upload-pack --v2; test $? = 129 && git-upload-pack

and then fallback to just "git-upload-pack". That would work without an
extra round-trip on real shell-capable servers, and eventually work on
restricted ones.

That doesn't help git://, though.

There are proposals floating around for basically easing into it with
config. Have a "remote.*.v2" option you can set locally to enable (or
disable) it. Default to "false". When there are enough v2 servers around
to make it worthwhile, flip the default to "auto" which will do the
probing (at some minor expense of handling fallbacks). Optionally we
could record the last response for "auto" and use that going forward.

> Another possibility, which would work for both HTTPS and
> git-protocol-over-TLS, would be to use ALPN.

Do people actually use git-over-TLS? There's no core support AFAIK, so
you'd have to hack it up with a client proxy and git-remote-ext.

For HTTPS, I'd just as soon use HTTP-level features.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] diff-highlight: add some tests.

[Junio C Hamano]

Lars Schneider  writes:

>> On 30 Jul 2016, at 17:11, Brian Henderson  wrote:
>> 
>> ---
>> contrib/diff-highlight/Makefile  |  5 ++
>> contrib/diff-highlight/t/Makefile| 19 +++
>> contrib/diff-highlight/t/t9400-diff-highlight.sh | 63 ++
>> contrib/diff-highlight/t/test-diff-highlight.sh  | 69 
>> 
>> 4 files changed, 156 insertions(+)
>> create mode 100644 contrib/diff-highlight/Makefile
>> create mode 100644 contrib/diff-highlight/t/Makefile
>> create mode 100644 contrib/diff-highlight/t/t9400-diff-highlight.sh
>> create mode 100644 contrib/diff-highlight/t/test-diff-highlight.sh
>> 
>> diff --git a/contrib/diff-highlight/Makefile 
>> b/contrib/diff-highlight/Makefile
>
> Would it make sense to add the contrib tests to the Travis-CI build, too?
> https://travis-ci.org/git/git/branches

The more the merrier ;-)?  I do not think of a downside, especially
if you are adding it as a separate thing that comes after the main
test, or for even better isolation, an entirely separate job.

By the way, how flaky are existing tests?  Are people actively
following breakage reports?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]

[Eric Wong]

Junio C Hamano  wrote:
> Stefan Beller  writes:
> > * Should the public-inbox offer another link to patches 1-n, without
> >   the cover letter? Or should it add instructions:
> >
> > If this is a patch series you can apply it locally as:
> > curl  >tmpXXX
> > git am tmpXXX && git am --skip && git am --continue

Currently for web users, I suggest:

curl $URL >tmpXXX

# open tmp and tag+copy to patchesXXX using MUA of choice:
# (also seems to be what Jeff describes):
mutt -f tmpXXX

git am patches

> I do not think it is sensible for "cover-letter" specific
> instructions.  However, I do not think it is unreasonable to either
> add another mbox.gz link or replace the behaviour of mbox.gz link so
> that you can grab a mbox that contains "this message and everything
> after it in the thread".  That way, I could open the first message,
> see something like this I found in your message:
> 
> >> Thread overview: 4+ messages in thread (expand / mbox.gz / Atom feed / 
> >> [top])
> >> 2016-08-15 23:06 Jacob Keller [this message]
> >> 2016-08-15 23:07 ` [PATCH v6 1/3] diff.c: remove output_prefix_length 
> >> field Jacob Keller
> >> 2016-08-15 23:07 ` [PATCH v6 2/3] graph: add support for --line-prefix on 
> >> all graph-aware output Jacob Keller
> >> 2016-08-15 23:07 ` [PATCH v6 3/3] diff: add SUBMODULE_DIFF format to 
> >> display submodule diff Jacob Keller
> 
> and then go to 1/3 and click that "this and everything that
> follows".

Adding more links might still fall down in cases where
fixup/squash patches are sent for specific patches in a series;
or when a v{N+1} series is posted in-reply-to an existing
series.

Perhaps adding checkbox next to each item might work as a
select-to-include-in-mbox download form.  However, I'm already
finding the lack of horizontal space disconcerting.

Maybe the -MM-DD could be shortened to MMDD.  It would
be closer to the date searching syntax used by mairix, as well
as the search enhancement I started working on earlier today:

  https://public-inbox.org/meta/20160816084926.29394-...@80x24.org/T/
  (still will deploy soonish)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 2/3] graph: add support for --line-prefix on all graph-aware output

[Jacob Keller]

On Tue, Aug 16, 2016 at 11:22 AM, Junio C Hamano  wrote:
> Jacob Keller  writes:
>
>> To make this work, we have to fix a few bugs in the graph API that force
>> graph_show_commit_msg to be used only when you have a valid graph.
>> Additionally, we extend the default_diff_output_prefix handler to work
>> even when no graph is enabled.
>>
>> This is somewhat of a hack on top of the graph API, but I think it
>> should be acceptable here.
>
> Unlike the opt-prefix-length I removed in 1/3, the length of the
> line-prefix will never change during the lifetime of a single
> diff_options struct, so it might turn out that repeated strlen()
> on it for each and every line output is wasteful.
>

I could change this to store the length at option initialization,
probably a good idea. Would it be better to just store it as a strbuf,
since these know their length already? Or just add a
line_prefix_length field? I agree that calling strlen a lot is
probably wasteful.

> Other than that, I didn't spot anything immediately questionable.
>

Thanks. I definitely feel like this is somewhat abusing the graph API
because we're adding something not exactly graph-related. But in some
ways it really is graph related as well, so I think it makes some
sense.

Regards,
Jake

> Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: upload-pack/ls-remote: showing non-HEAD symbolic refs?

[Josh Triplett]

On Tue, Aug 16, 2016 at 11:50:11AM -0700, Stefan Beller wrote:
> On Tue, Aug 16, 2016 at 11:28 AM, Jeff King  wrote:
> > On Tue, Aug 16, 2016 at 10:34:44AM -0700, Josh Triplett wrote:
> >
> >> > Sadly you cannot use a capability to fix that, because all of this
> >> > happens before the client agrees to any capabilities (you can find
> >> > discussion of a "v2" protocol on the list which solves this, but it's
> >> > sort of languishing in the design phase).
> >>
> >> As a potential 1.1 version, which could work in a backward-compatible
> >> way with existing servers and no additional round-trip: what if, in the
> >> smart HTTP protocol, the client advertised client capabilities with an
> >> additional HTTP header (e.g.  "Git-Client-Caps: symrefs othershiny
> >> featurenames"?  git-http-backend could then pass those capabilities to
> >> git-upload-pack (--client-caps='...'), which could take them into
> >> account in the initial response?
> >>
> >> That wouldn't work as a single-pass approach for SSH, since the client
> >> can't know if the server's upload-pack supports --client-caps, but it
> >> would work for the smart HTTP protocol.
> >
> > You can dig up the discussion on the list under the name "protocol v2",
> > but basically yes, that approach has been considered. It's a little
> > gross just because it leaves other protocols behind http (and it is not
> > necessarily a good idea to push people into http, because it has some
> > fundamental drawbacks over the other protocols because of its
> > half-duplex nature).
> 
> Some more thoughts on protocol v2 (the good parts to be attributed to
> jrnie...@gmail.com):
> 
> * In case of http we can use the http header and know information
>   about the client, i.e. if it may support the following ideas:
> 
> * Instead of introducing a new protocol we introduce a capability\
>   "resend-ref-advertisement" and only advertise very few refs (e.g.
>   only the branches, not the pending changes in case of Gerrit)
> 
> * The client can then ignore the refs advertisement and ask for a resend
>   of the refs with more specification,
>   e.g. "want refs/heads/*", so allowing more than just sha1s in the
>   want line but complex things like branch patterns.

That seems like something a client could advertise via client
capabilities.  For instance, if the client doesn't want the initial refs
list, only the server capabilities, it could say "capsonly".  (It might
also be reasonable to cache the knowledge that a server supports client
capabilities, though not any specific capability.)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Prefer "long" key format output when verifying pgp signatures

[Linus Torvalds]

From: Linus Torvalds 
Date: Tue, 16 Aug 2016 13:10:24 -0700
Subject: [PATCH] Prefer "long" key format output when verifying pgp signatures

Yes, gpg2 already uses the long format by default, but most
distributions seem to still have "gpg" be the older 1.x version due to
compatibility reasons.  And older versions of gpg only show the 32-bit
short ID, which is quite insecure.

This doesn't actually matter for the _verification_ itself: if the
verification passes, the pgp signature is good.  But if you don't
actually have the key yet, and want to fetch it, or you want to check
exactly which key was used for verification and want to check it, we
should specify the key with more precision.

In fact, we should preferentially specify the whole key fingerprint, but 
gpg doesn't actually support that.  Which is really quite sad.

Showing the "long" format improves things to at least show 64 bits of
the fingerprint.  That's a lot better, even if it's not perfect.

This change the log format for "git log --show-signature" from

commit 2376d31787760af598db23bb3982a57419854e5c
merged tag 'v2.9.3'
gpg: Signature made Fri 12 Aug 2016 09:17:59 AM PDT using RSA key ID 
96AFE6CB
gpg: Good signature from "Junio C Hamano "
gpg: aka "Junio C Hamano "
gpg: aka "Junio C Hamano "
Merge: 2807cd7b25af e0c1ceafc5be
Author: Junio C Hamano 
Date:   Fri Aug 12 10:02:18 2016 -0700

to

commit 2376d31787760af598db23bb3982a57419854e5c
merged tag 'v2.9.3'
gpg: Signature made Fri 12 Aug 2016 09:17:59 AM PDT
gpg:using RSA key B0B5E88696AFE6CB
gpg: Good signature from "Junio C Hamano "
gpg: aka "Junio C Hamano "
gpg: aka "Junio C Hamano "
Merge: 2807cd7b25af e0c1ceafc5be
Author: Junio C Hamano 
Date:   Fri Aug 12 10:02:18 2016 -0700

(note the longer key ID, but also the reflowing of the text) and also 
changes the format in the merge messages when merging a signed 
tag.

If you already use gpg2 (either because it's installed by default, or 
because you have set your gpg_program configuration to point to gpg2), 
that already used the long format, you'll also see a change: it will now 
have the same formatting as gpg 1.x, and the verification string looks 
something like

gpg: Signature made Sun 24 Jul 2016 12:24:02 PM PDT
gpg:using RSA key 79BE3E4300411886
gpg: Good signature from "Linus Torvalds " 
[ultimate]

where it used to be on one line:

gpg: Signature made Sun 24 Jul 2016 12:24:02 PM PDT using RSA key ID 
79BE3E4300411886
gpg: Good signature from "Linus Torvalds " 
[ultimate]

so there is certainly a chance this could break some automated scripting.  
But the 32-bit key ID's really are broken. Also note that because of the 
differences between gpg-1.x and gpg-2.x, hopefully any scripted key ID 
parsing code (if such code exists) is already flexible enough to not care.

This was triggered by the fact that the "evil32" project keys ended up
leaking to the public key servers, so now there are 32-bit aliases for
just about every open source developer that you can easily get by
mistake if you use the 32-bit short ID format.

Signed-off-by: Linus Torvalds 
---

That's a very long commit message for a very trivial patch.

I'm not particularly happy with the 64-bit long format either, but it's 
better than what we have now, and appears to be as good as it gets.

 gpg-interface.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gpg-interface.c b/gpg-interface.c
index 08356f92e7b3..8672edaf4823 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -217,6 +217,7 @@ int verify_signed_buffer(const char *payload, size_t 
payload_size,
argv_array_pushl(,
 gpg_program,
 "--status-fd=1",
+"--keyid-format=long",
 "--verify", temp.filename.buf, "-",
 NULL);
 
-- 
2.10.0.rc0.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: upload-pack/ls-remote: showing non-HEAD symbolic refs?

[Josh Triplett]

On Tue, Aug 16, 2016 at 02:28:52PM -0400, Jeff King wrote:
> On Tue, Aug 16, 2016 at 10:34:44AM -0700, Josh Triplett wrote:
> 
> > > Sadly you cannot use a capability to fix that, because all of this
> > > happens before the client agrees to any capabilities (you can find
> > > discussion of a "v2" protocol on the list which solves this, but it's
> > > sort of languishing in the design phase).
> > 
> > As a potential 1.1 version, which could work in a backward-compatible
> > way with existing servers and no additional round-trip: what if, in the
> > smart HTTP protocol, the client advertised client capabilities with an
> > additional HTTP header (e.g.  "Git-Client-Caps: symrefs othershiny
> > featurenames"?  git-http-backend could then pass those capabilities to
> > git-upload-pack (--client-caps='...'), which could take them into
> > account in the initial response?
> > 
> > That wouldn't work as a single-pass approach for SSH, since the client
> > can't know if the server's upload-pack supports --client-caps, but it
> > would work for the smart HTTP protocol.
> 
> You can dig up the discussion on the list under the name "protocol v2",
> but basically yes, that approach has been considered. It's a little
> gross just because it leaves other protocols behind http (and it is not
> necessarily a good idea to push people into http, because it has some
> fundamental drawbacks over the other protocols because of its
> half-duplex nature).

I looked through the "protocol v2" threads, but couldn't find any
discussions of using HTTP headers.  I found some mentions of using
additional query parameters on the git-upload-pack request, which would
break compatibility with existing servers (they won't just ignore the
extra parameter).

--client-caps could work for SSH as well, it just requires an extra
round-trip to check for --client-caps.  Call git-upload-pack
--supports-client-caps, ignore any output (which with current git will
consist of a usage message), see if it returns a 0 exit code, if so,
call git-upload-pack --client-caps='...', and if not just call
git-upload-pack.  (A new git-upload-pack-2 binary would also work, but
that seems like overkill.)  I don't see any way around the extra round
trip here that would preserve backward compatibility with existing SSH
servers (which may force clients to *only* run exactly the command
"git-upload-pack" and nothing else).

Another possibility, which would work for both HTTPS and
git-protocol-over-TLS, would be to use ALPN.

git:// , however, would require a new service name, and that service
would have to accept client caps in-band.  That doesn't seem nearly as
important to support, though.  And that approach doesn't seem preferable
for HTTP, which would require modification to server configuration to
support the new path rather than an HTTP header to the existing path.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] rev-parse: respect core.hooksPath in --git-path

[Junio C Hamano]

Remi Galan Alfonso 
writes:

> Johannes Schindelin  writes:
>> Hi Rémi,
>> 
>> On Tue, 16 Aug 2016, Remi Galan Alfonso wrote:
>> 
>> > Johannes Schindelin  writes:
>> > > diff --git a/t/t1350-config-hooks-path.sh b/t/t1350-config-hooks-path.sh
>> > > index 5e3fb3a..f1f9aee 100755
>> > > --- a/t/t1350-config-hooks-path.sh
>> > > +++ b/t/t1350-config-hooks-path.sh
>> > > @@ -34,4 +34,10 @@ test_expect_success 'Check that various forms of 
>> > > specifying core.hooksPath work'
>> > >  test_cmp expect actual
>> > >  '
>> > >  
>> > > +test_expect_success 'git rev-parse --git-path hooks' '
>> > > +git config core.hooksPath .git/custom-hooks &&
>> >
>> > Any reason to not use 'test_config' here?
>> 
>> Yes: consistency. The rest of the script uses `git config`, not
>> `test_config`.
>
> Fine by me, then. Sorry for the noise.

No, thanks for reviewing.  I'll take Dscho's patch as-is but once it
hits 'next', it probably is a good idea to do a separate clean-up
patch on top to use test_config where necessary.

Having said that, this entire script is about constantly changing
the value of that single configuration variable and see how the code
performs, so any new test added after existing ones is expected to
ignore left-over values in the variable and set it to a value of its
own liking.  So I suspect there is no existing "git config" call in
this script, with or without Dscho's patch, that would benefit from
getting converted to test_config.


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 3/3] diff: add SUBMODULE_DIFF format to display submodule diff

[Jacob Keller]

On Tue, Aug 16, 2016 at 11:48 AM, Junio C Hamano  wrote:
> Jacob Keller  writes:
>
>> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
>> index d5a5b17d5088..f5d693afad6c 100644
>> --- a/Documentation/diff-config.txt
>> +++ b/Documentation/diff-config.txt
>> @@ -123,7 +123,8 @@ diff.suppressBlankEmpty::
>>  diff.submodule::
>>   Specify the format in which differences in submodules are
>>   shown.  The "log" format lists the commits in the range like
>> - linkgit:git-submodule[1] `summary` does.  The "short" format
>> + linkgit:git-submodule[1] `summary` does.  The "diff" format shows the
>> + diff between the beginning and end of the range. The "short" format
>>   format just shows the names of the commits at the beginning
>>   and end of the range.  Defaults to short.
>
> It would be much better to describe the default one first and then
> more involved one next, no?  That would also match what the change
> to "diff-options" in this patch does (can be seen below).
>

The main thing is that "--submodule" alone means "use the log format"
so I think that's why it went first. I can reword this to make it more
clear how this works.

Thanks,
Jake

>> @@ -2311,6 +2322,15 @@ static void builtin_diff(const char *name_a,
>>   two->dirty_submodule,
>>   meta, del, add, reset);
>>   return;
>> + } else if (o->submodule_format == DIFF_SUBMODULE_DIFF &&
>> +(!one->mode || S_ISGITLINK(one->mode)) &&
>> +(!two->mode || S_ISGITLINK(two->mode))) {
>> + show_submodule_diff(o->file, one->path ? one->path : two->path,
>> + line_prefix,
>> + one->oid.hash, two->oid.hash,
>> + two->dirty_submodule,
>> + meta, a_prefix, b_prefix, reset);
>> + return;
>>   }
>
> The "either missing or is submodule" check used here is being
> consistent with the existing "submodule=log" case.  Good.
>
>> diff --git a/submodule.c b/submodule.c
>> index 1b5cdfb7e784..b1da68dd49c9 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -333,6 +333,136 @@ static void print_submodule_summary(struct rev_info 
>> *rev, FILE *f,
>>   strbuf_release();
>>  }
>>
>> +void show_submodule_diff(FILE *f, const char *path,
>> + const char *line_prefix,
>> + unsigned char one[20], unsigned char two[20],
>> + unsigned dirty_submodule, const char *meta,
>> + const char *a_prefix, const char *b_prefix,
>> + const char *reset)
>> +{
>> + struct strbuf submodule_git_dir = STRBUF_INIT, sb = STRBUF_INIT;
>> + struct child_process cp = CHILD_PROCESS_INIT;
>> + const char *git_dir;
>> +
>> + if (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) {
>> + fprintf(f, "%sSubmodule %s contains untracked content\n",
>> + line_prefix, path);
>> + }
>> + if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED) {
>> + fprintf(f, "%sSubmodule %s contains modified content\n",
>> + line_prefix, path);
>> + }
>> +
>> + strbuf_addf(, "%s%sSubmodule %s %s..",
>> + line_prefix, meta, path,
>> + find_unique_abbrev(one, DEFAULT_ABBREV));
>> + strbuf_addf(, "%s:%s",
>> + find_unique_abbrev(two, DEFAULT_ABBREV),
>> + reset);
>> + fwrite(sb.buf, sb.len, 1, f);
>> +
>> + if (is_null_sha1(one))
>> + fprintf(f, " (new submodule)");
>> + if (is_null_sha1(two))
>> + fprintf(f, " (submodule deleted)");
>
> These messages are in sync with show_submodule_summary() that is
> used in --submodule=log codepath.  Good.
>

They're not exactly the same due to some ways of splitting up new lines.

>> + /*
>> +  * We need to determine the most accurate location to call the sub
>> +  * command, and handle the various corner cases involved. We'll first
>> +  * attempt to use the path directly if the submodule is checked out.
>> +  * Then, if that fails, we'll check the standard module location in
>> +  * the git directory. If even this fails, it means we can't do the
>> +  * lookup because the module has not been initialized.
>> +  */
>
> This is more elaborate than what show_submodule_summary() does,
> isn't it?  Would it make the patch series (and the resulting code)
> more understandable if you used the same code by refactoring these
> two?  If so, I wonder if it makes sense to split 3/3 into a few
> separate steps:

The show_submodule_summary just uses "add_submodule_odb" which adds
the submodule as an alternate source of objects, if I understand
correctly.

>
>  * Update the internal "--submodule=" handling without adding
>the "--submodule=diff" and show_submodule_diff() function.
>
>  * 

Re: Working with zip files

[Junio C Hamano]

Jakub Narębski  writes:

> There is also `textconv` filter that can be used instead; it might
> be 'unzip -c' (extract files to stdout, with filenames), or 'unzip -p'
> (same, without filenames).

That assumes that the in-repository data is zipped binary blob; the
result won't delta well, will it?


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Working with zip files

[Jakub Narębski]

W dniu 16.08.2016 o 18:58, Junio C Hamano pisze:
> David Lang  writes:
> 
>> you should be able to use clean/smudge to have git store the files
>> uncompressed, which will help a lot.

You can find rezip clean/smudge filter (originally intended for
OpenDocument Format (ODF), that is OpenOffice.org etc.) that stores
zip or zip-archive (like ODT, jar, etc.) uncompressed.  I think
you can find it on GitWiki, but I might be mistaken.

>> I think there's a way to tell it to do a xml aware diff/patch, but I
>> don't remember how.
> 
> I do not know about "patch" (in the sense of "git apply"), but "git
> diff" (and "git log -p") can take advantage of the clean/smudge
> mechanism.  I used to deal with a file format that is gzipped xml so
> my clean filter was "gzip -dc" while the smudge was "gzip -cn".
> Essentially, this stores the xml before compression in the repository
> so blobs delta well with each other and also the revisions are
> made textually diff-able.
> 
> Nikolaus's case has one extra layer of complexity in that the "file"
> is actually an archive of multiple files.  The clean/smudge pair he
> writes need to be a filter that flattens the archive into a single
> human-readable text byte stream and its reverse.

There is also `textconv` filter that can be used instead; it might
be 'unzip -c' (extract files to stdout, with filenames), or 'unzip -p'
(same, without filenames).

-- 
Jakub Narębski
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] help: make option --help open man pages only for Git commands

[Junio C Hamano]

Ralf Thielow  writes:

> 2016-08-16 19:27 GMT+02:00 Junio C Hamano :
>> Ralf Thielow  writes:
>>> +
>>> + if (swapped)
>>> + return help_unknown_cmd(cmd);
>>
>> I am guilty of suggesting "swapped"; even if we are going to mark
>> this as OPT_HIDDEN, I think we should be able to think of a better
>> name.  I think the meaning of this boolean is "we know that this is
>> not a guide and is meant to be a command.", and I hope we can come
>> up with a name that concisely expresses that (e.g. "--not-a-guide",
>> "--must-be-a-command").
>>
>
> I think "--cmd-only" is a good name.  With a good name I think it's worth
> to make this option visible to the user.

Sure.  I'd prefer "cmd" to be spelled out in anything that is
end-user facing, though.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: upload-pack/ls-remote: showing non-HEAD symbolic refs?

[Jeff King]

On Tue, Aug 16, 2016 at 10:34:44AM -0700, Josh Triplett wrote:

> > Sadly you cannot use a capability to fix that, because all of this
> > happens before the client agrees to any capabilities (you can find
> > discussion of a "v2" protocol on the list which solves this, but it's
> > sort of languishing in the design phase).
> 
> As a potential 1.1 version, which could work in a backward-compatible
> way with existing servers and no additional round-trip: what if, in the
> smart HTTP protocol, the client advertised client capabilities with an
> additional HTTP header (e.g.  "Git-Client-Caps: symrefs othershiny
> featurenames"?  git-http-backend could then pass those capabilities to
> git-upload-pack (--client-caps='...'), which could take them into
> account in the initial response?
> 
> That wouldn't work as a single-pass approach for SSH, since the client
> can't know if the server's upload-pack supports --client-caps, but it
> would work for the smart HTTP protocol.

You can dig up the discussion on the list under the name "protocol v2",
but basically yes, that approach has been considered. It's a little
gross just because it leaves other protocols behind http (and it is not
necessarily a good idea to push people into http, because it has some
fundamental drawbacks over the other protocols because of its
half-duplex nature).

> > That should Just Work over the existing http protocol without requiring
> > an extra request.
> 
> It'd require one extra request for git ls-remote, which normally doesn't
> need the second round-trip, but that still seems reasonable.

Good point. I don't think there's an easy way around that short of v2 or
v1.1 that you mention above. I agree it's probably reasonable, though.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 3/3] diff: add SUBMODULE_DIFF format to display submodule diff

[Junio C Hamano]

Jacob Keller  writes:

> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
> index d5a5b17d5088..f5d693afad6c 100644
> --- a/Documentation/diff-config.txt
> +++ b/Documentation/diff-config.txt
> @@ -123,7 +123,8 @@ diff.suppressBlankEmpty::
>  diff.submodule::
>   Specify the format in which differences in submodules are
>   shown.  The "log" format lists the commits in the range like
> - linkgit:git-submodule[1] `summary` does.  The "short" format
> + linkgit:git-submodule[1] `summary` does.  The "diff" format shows the
> + diff between the beginning and end of the range. The "short" format
>   format just shows the names of the commits at the beginning
>   and end of the range.  Defaults to short.

It would be much better to describe the default one first and then
more involved one next, no?  That would also match what the change
to "diff-options" in this patch does (can be seen below).

> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index cc4342e2034d..d3ca4ad2c2c5 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -215,8 +215,11 @@ any of those replacements occurred.
>   the commits in the range like linkgit:git-submodule[1] `summary` does.
>   Omitting the `--submodule` option or specifying `--submodule=short`,
>   uses the 'short' format. This format just shows the names of the commits
> - at the beginning and end of the range.  Can be tweaked via the
> - `diff.submodule` configuration variable.
> + at the beginning and end of the range. When `--submodule=diff` is
> + given, the 'diff' format is used. This format shows the diff between
> + the old and new submodule commmit from the perspective of the
> + submodule.  Defaults to `diff.submodule` or 'short' if the config
> + option is unset.

> @@ -2311,6 +2322,15 @@ static void builtin_diff(const char *name_a,
>   two->dirty_submodule,
>   meta, del, add, reset);
>   return;
> + } else if (o->submodule_format == DIFF_SUBMODULE_DIFF &&
> +(!one->mode || S_ISGITLINK(one->mode)) &&
> +(!two->mode || S_ISGITLINK(two->mode))) {
> + show_submodule_diff(o->file, one->path ? one->path : two->path,
> + line_prefix,
> + one->oid.hash, two->oid.hash,
> + two->dirty_submodule,
> + meta, a_prefix, b_prefix, reset);
> + return;
>   }

The "either missing or is submodule" check used here is being
consistent with the existing "submodule=log" case.  Good.

> diff --git a/submodule.c b/submodule.c
> index 1b5cdfb7e784..b1da68dd49c9 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -333,6 +333,136 @@ static void print_submodule_summary(struct rev_info 
> *rev, FILE *f,
>   strbuf_release();
>  }
>  
> +void show_submodule_diff(FILE *f, const char *path,
> + const char *line_prefix,
> + unsigned char one[20], unsigned char two[20],
> + unsigned dirty_submodule, const char *meta,
> + const char *a_prefix, const char *b_prefix,
> + const char *reset)
> +{
> + struct strbuf submodule_git_dir = STRBUF_INIT, sb = STRBUF_INIT;
> + struct child_process cp = CHILD_PROCESS_INIT;
> + const char *git_dir;
> +
> + if (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) {
> + fprintf(f, "%sSubmodule %s contains untracked content\n",
> + line_prefix, path);
> + }
> + if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED) {
> + fprintf(f, "%sSubmodule %s contains modified content\n",
> + line_prefix, path);
> + }
> +
> + strbuf_addf(, "%s%sSubmodule %s %s..",
> + line_prefix, meta, path,
> + find_unique_abbrev(one, DEFAULT_ABBREV));
> + strbuf_addf(, "%s:%s",
> + find_unique_abbrev(two, DEFAULT_ABBREV),
> + reset);
> + fwrite(sb.buf, sb.len, 1, f);
> +
> + if (is_null_sha1(one))
> + fprintf(f, " (new submodule)");
> + if (is_null_sha1(two))
> + fprintf(f, " (submodule deleted)");

These messages are in sync with show_submodule_summary() that is
used in --submodule=log codepath.  Good.

> + /*
> +  * We need to determine the most accurate location to call the sub
> +  * command, and handle the various corner cases involved. We'll first
> +  * attempt to use the path directly if the submodule is checked out.
> +  * Then, if that fails, we'll check the standard module location in
> +  * the git directory. If even this fails, it means we can't do the
> +  * lookup because the module has not been initialized.
> +  */

This is more elaborate than what show_submodule_summary() does,

Re: upload-pack/ls-remote: showing non-HEAD symbolic refs?

[Stefan Beller]

On Tue, Aug 16, 2016 at 11:28 AM, Jeff King  wrote:
> On Tue, Aug 16, 2016 at 10:34:44AM -0700, Josh Triplett wrote:
>
>> > Sadly you cannot use a capability to fix that, because all of this
>> > happens before the client agrees to any capabilities (you can find
>> > discussion of a "v2" protocol on the list which solves this, but it's
>> > sort of languishing in the design phase).
>>
>> As a potential 1.1 version, which could work in a backward-compatible
>> way with existing servers and no additional round-trip: what if, in the
>> smart HTTP protocol, the client advertised client capabilities with an
>> additional HTTP header (e.g.  "Git-Client-Caps: symrefs othershiny
>> featurenames"?  git-http-backend could then pass those capabilities to
>> git-upload-pack (--client-caps='...'), which could take them into
>> account in the initial response?
>>
>> That wouldn't work as a single-pass approach for SSH, since the client
>> can't know if the server's upload-pack supports --client-caps, but it
>> would work for the smart HTTP protocol.
>
> You can dig up the discussion on the list under the name "protocol v2",
> but basically yes, that approach has been considered. It's a little
> gross just because it leaves other protocols behind http (and it is not
> necessarily a good idea to push people into http, because it has some
> fundamental drawbacks over the other protocols because of its
> half-duplex nature).

Some more thoughts on protocol v2 (the good parts to be attributed to
jrnie...@gmail.com):

* In case of http we can use the http header and know information
  about the client, i.e. if it may support the following ideas:

* Instead of introducing a new protocol we introduce a capability\
  "resend-ref-advertisement" and only advertise very few refs (e.g.
  only the branches, not the pending changes in case of Gerrit)

* The client can then ignore the refs advertisement and ask for a resend
  of the refs with more specification,
  e.g. "want refs/heads/*", so allowing more than just sha1s in the
  want line but complex things like branch patterns.

>
>> > That should Just Work over the existing http protocol without requiring
>> > an extra request.
>>
>> It'd require one extra request for git ls-remote, which normally doesn't
>> need the second round-trip, but that still seems reasonable.
>
> Good point. I don't think there's an easy way around that short of v2 or
> v1.1 that you mention above. I agree it's probably reasonable, though.
>
> -Peff
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 2/3] graph: add support for --line-prefix on all graph-aware output

[Junio C Hamano]

Jacob Keller  writes:

> To make this work, we have to fix a few bugs in the graph API that force
> graph_show_commit_msg to be used only when you have a valid graph.
> Additionally, we extend the default_diff_output_prefix handler to work
> even when no graph is enabled.
>
> This is somewhat of a hack on top of the graph API, but I think it
> should be acceptable here.

Unlike the opt-prefix-length I removed in 1/3, the length of the
line-prefix will never change during the lifetime of a single
diff_options struct, so it might turn out that repeated strlen()
on it for each and every line output is wasteful.

Other than that, I didn't spot anything immediately questionable.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 1/3] diff.c: remove output_prefix_length field

[Jacob Keller]

On Tue, Aug 16, 2016 at 11:03 AM, Junio C Hamano  wrote:
> Jacob Keller  writes:
>
>> From: Junio C Hamano 
>
> Thanks.  I had quite a many typoes in this one.
>
>
> No need to resend; I had it with the above fix in 'pu' for a few
> days already ;-)

Thanks,
Jake
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 1/3] diff.c: remove output_prefix_length field

[Junio C Hamano]

Jacob Keller  writes:

> From: Junio C Hamano 

Thanks.  I had quite a many typoes in this one.

> "diff/log --stat" has a logic that determines the display columns
> available for the diffstat part of the output and apportions it for
> pathnames and diffstat graph automatically.
>
> 5e71a84a (Add output_prefix_length to diff_options, 2012-04-16)
> added the output_prefix_length field to diff_options structure to
> allow this logic subtract the display columns used for display the

s/^.*$/allow this logic to subtract the display columns used for the/

> history graph part from the total "terminal width"; this matters
> when the "git log --graph -p" option is in use.
>
> The field be set to the number of display columns needed to show the

s/field be/field must be/

> output from the output_prefix() callback.  Any new output_prefix()
> callback must also update the field accordingly, which is error

"output from the output_prefix() callback, which is error"

> prone.

No need to resend; I had it with the above fix in 'pu' for a few
days already ;-)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] help: make option --help open man pages only for Git commands

[Ralf Thielow]

2016-08-16 19:27 GMT+02:00 Junio C Hamano :
> Ralf Thielow  writes:
>> +
>> + if (swapped)
>> + return help_unknown_cmd(cmd);
>
> I am guilty of suggesting "swapped"; even if we are going to mark
> this as OPT_HIDDEN, I think we should be able to think of a better
> name.  I think the meaning of this boolean is "we know that this is
> not a guide and is meant to be a command.", and I hope we can come
> up with a name that concisely expresses that (e.g. "--not-a-guide",
> "--must-be-a-command").
>

I think "--cmd-only" is a good name.  With a good name I think it's worth
to make this option visible to the user.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]

[Junio C Hamano]

Jeff King  writes:

> For my workflow, it is not about "initial skip", but rather just "skip
> emails that don't have patches in them at all".

OK.  That is different from "the subject line says 0/N so let's
skip".

If we can safely determine that there is no patch in a message,
skipping it may feel sensible.  Historically, we try to err on the
safe side by stopping when we do not understand an input and that is
where "Patch is empty" message came from, because skipping and
continuing, even with a warning, while applying tons of patches is
not good enough to get user's attention.

An "ignore empty input" option (and eventually with a configuration)
may not be a bad idea.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]

[Junio C Hamano]

Stefan Beller  writes:

> In your work flow, how do you respect the cover letter?
> e.g. in 3787e3c16ced:
>
> Merge branch 'ew/http-backend-batch-headers'
>
> The http-backend (the server-side component of smart-http
> transport) used to trickle the HTTP header one at a time.  Now
> these write(2)s are batched.
>
> * ew/http-backend-batch-headers:
>   http-backend: buffer headers before sending
>
> Is the text from the original author (and if so from which version
> of the cover letter) or is it your work?

The source of truth in the merge log message is the "What's cooking"
report.  I really prefer to write these in my own words, as that is
a good yardstick to measure how much/little I understand the topic.
If I cannot describe it concisely, in a way suitable as an entry in
the release notes, that means I am merging a topic I do not have a
good idea about, which is quite irresponsive.  Forcing me to write
these myself keeps me honest.

Of course, if a cover letter describes the topic well, it would help
me write the entry in the "What's cooking" report.

It is a bit tricky to aim for the automation, though.  The cover is
an overview of the proposed log messages and typically tells a story
"I do this, and then this, and finally that", plus a reroll-specific
commentary like "what changed since the last round".  On the other
hand, the entries in the release notes gives a description of what
happened from a third-party's point of view.  They are told in
different voice for different target audience.


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: upload-pack/ls-remote: showing non-HEAD symbolic refs?

[Josh Triplett]

On Tue, Aug 16, 2016 at 12:31:45PM -0400, Jeff King wrote:
> On Tue, Aug 16, 2016 at 09:18:39AM -0700, Josh Triplett wrote:
> 
> > Commit 5e7dcad771cb873e278a0571b46910d7c32e2f6c in September 2013 added
> > support to upload-pack to show the symbolic target of non-HEAD symbolic
> > refs.  However, commit d007dbf7d6d647dbcf0f357545f43f36dec46f3b in
> > November 2013 reverted that, because it used a capability to transmit
> > the information, and capabilities have a limited size (limited by the
> > pkt-line format which can't send lines longer than 64k) and can't
> > transmit an arbitrary number of symrefs.
> > 
> > (Incidentally, couldn't the same problem occur if the HEAD points to a
> > long enough path to exceed 64k?
> 
> Yes. But it's a lot easier to say "your 64k symref is ridiculous; don't
> do that" than it is to say "oh, you happened to have a lot of symrefs in
> your repository, so we overflowed and failed".
> 
> Besides which, the whole protocol cannot handle refnames larger than
> 64k, so it's not a new problem.

Absolutely agreed.  I mentioned it only because if a server provided any
mechanism to send it symbolic ref targets, this could lead to a
situation where you could push a repository that you could not
subsequently pull.  Depending on the protocols involved, that could
potentially require manual admin intervention, or could even result in a
DoS.

Not something that can arise with git itself, as send-pack/receive-pack
doesn't support sending symbolic ref targets, but I could imagine a
server doing so to allow setting HEAD.

> > I'd like to be able to see the targets of non-HEAD symbolic refs for a
> > repository (symbolic refs under refs/).  I'm interested in extending
> > upload-pack to expose those somehow.  What seems like a sensible format
> > to do so?
> > 
> > Would it make sense to advertise a new capability for symbolic ref
> > targets, which would allow the client to send back a dedicated request
> > for the targets of all symrefs?
> 
> It will definitely require a new capability. You cannot just send a
> "\0symref=..." trailer after each ref, because older clients treat
> multiple "\0" trailers as overwriting one another (so it essentially
> overwrites the old capabilities).
> 
> Sadly you cannot use a capability to fix that, because all of this
> happens before the client agrees to any capabilities (you can find
> discussion of a "v2" protocol on the list which solves this, but it's
> sort of languishing in the design phase).

As a potential 1.1 version, which could work in a backward-compatible
way with existing servers and no additional round-trip: what if, in the
smart HTTP protocol, the client advertised client capabilities with an
additional HTTP header (e.g.  "Git-Client-Caps: symrefs othershiny
featurenames"?  git-http-backend could then pass those capabilities to
git-upload-pack (--client-caps='...'), which could take them into
account in the initial response?

That wouldn't work as a single-pass approach for SSH, since the client
can't know if the server's upload-pack supports --client-caps, but it
would work for the smart HTTP protocol.

> So you are stuck introducing a new phase into the protocol, which is
> probably rather tricky (especially with the http protocol, which is very
> sensitive to extra round-trips). I guess the least-invasive way would be
> to communicate the desires in the "want" phase, and then have the server
> dump it out with the packfile. Like:
> 
>   - server claims "I support symref-wants" in the capability phase
> 
>   - during the negotiation phase, in addition to "want" and "have", the
> client may send "symref " packets. Probably  should be a
> wildcard to avoid having to ask about each ref individually.
> 
>   - before outputting the packfile in the final phase, if any "symref"
> wants were sent by the client, the server dumps a list of "symref
>  " packets, followed by a flush packet.
> 
> That should Just Work over the existing http protocol without requiring
> an extra request.

It'd require one extra request for git ls-remote, which normally doesn't
need the second round-trip, but that still seems reasonable.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]

[Stefan Beller]

On Tue, Aug 16, 2016 at 10:10 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> So as a discussion starter:
>> * Should git am skip a patch 00/XX automatically ?
>
> No.  My preference is to add "--initial-skip=", though.
>
> When I receive a patch series to reroll another series, I somehow
> know and verify that earlier N patches have not changed, I detach
> the HEAD at the last unchanged commit from the previous round and
> apply the remainder of the new series, so that I can preserve the
> author timestamps of earlier steps from the previous series.  By
> the time I "know and verify" where the first step that was updated,
> I have a full series in a single mbox; having "--initial-skip="
> would help with that use case, too, and "skipping the first" is a
> narrow special case of giving N=1.

In your work flow, how do you respect the cover letter?
e.g. in 3787e3c16ced:

Merge branch 'ew/http-backend-batch-headers'

The http-backend (the server-side component of smart-http
transport) used to trickle the HTTP header one at a time.  Now
these write(2)s are batched.

* ew/http-backend-batch-headers:
  http-backend: buffer headers before sending

Is the text from the original author (and if so from which version
of the cover letter) or is it your work?

>
>> * Should the public-inbox offer another link to patches 1-n, without
>>   the cover letter? Or should it add instructions:
>>
>> If this is a patch series you can apply it locally as:
>> curl  >tmpXXX
>> git am tmpXXX && git am --skip && git am --continue
>
> I do not think it is sensible for "cover-letter" specific
> instructions.  However, I do not think it is unreasonable to either
> add another mbox.gz link or replace the behaviour of mbox.gz link so
> that you can grab a mbox that contains "this message and everything
> after it in the thread".  That way, I could open the first message,
> see something like this I found in your message:
>
>>> Thread overview: 4+ messages in thread (expand / mbox.gz / Atom feed / 
>>> [top])
>>> 2016-08-15 23:06 Jacob Keller [this message]
>>> 2016-08-15 23:07 ` [PATCH v6 1/3] diff.c: remove output_prefix_length field 
>>> Jacob Keller
>>> 2016-08-15 23:07 ` [PATCH v6 2/3] graph: add support for --line-prefix on 
>>> all graph-aware output Jacob Keller
>>> 2016-08-15 23:07 ` [PATCH v6 3/3] diff: add SUBMODULE_DIFF format to 
>>> display submodule diff Jacob Keller
>
> and then go to 1/3 and click that "this and everything that
> follows".

Both thoughts are sensible; However the --initial-skip=
doesn't address the special case of storing the cover letter
(which we eventually want to do?)

I thought of it as the following with room for improvement:

diff --git a/builtin/am.c b/builtin/am.c (shite space broken):
index 739b34d..5f08b61 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1246,6 +1246,7 @@ static int parse_mail(struct am_state *state,
const char *mail)
FILE *fp;
struct strbuf sb = STRBUF_INIT;
struct strbuf msg = STRBUF_INIT;
+   struct strbuf subject = STRBUF_INIT;
struct strbuf author_name = STRBUF_INIT;
struct strbuf author_date = STRBUF_INIT;
struct strbuf author_email = STRBUF_INIT;
@@ -1309,6 +1310,7 @@ static int parse_mail(struct am_state *state,
const char *mail)
if (msg.len)
strbuf_addch(, '\n');
strbuf_addstr(, x);
+   strbuf_addstr(, x);
} else if (skip_prefix(sb.buf, "Author: ", ))
strbuf_addstr(_name, x);
else if (skip_prefix(sb.buf, "Email: ", ))
@@ -1325,8 +1327,17 @@ static int parse_mail(struct am_state *state,
const char *mail)
}

if (is_empty_file(am_path(state, "patch"))) {
-   printf_ln(_("Patch is empty. Was it split wrong?"));
-   die_user_resolve(state);
+   if (indicates_coverletter())
+   /*
+* TODO: store the cover letter as the first or last
+* commit or as branch..description
+*/
+   ret = 1;
+   goto finish;
+   else {
+   printf_ln(_("Patch is empty. Was it split wrong?"));
+   die_user_resolve(state);
+   }
}

strbuf_addstr(, "\n\n");
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]

[Jeff King]

On Tue, Aug 16, 2016 at 10:10:42AM -0700, Junio C Hamano wrote:

> Stefan Beller  writes:
> 
> > So as a discussion starter:
> > * Should git am skip a patch 00/XX automatically ?
> 
> No.  My preference is to add "--initial-skip=", though.
> 
> When I receive a patch series to reroll another series, I somehow
> know and verify that earlier N patches have not changed, I detach
> the HEAD at the last unchanged commit from the previous round and
> apply the remainder of the new series, so that I can preserve the
> author timestamps of earlier steps from the previous series.  By
> the time I "know and verify" where the first step that was updated,
> I have a full series in a single mbox; having "--initial-skip="
> would help with that use case, too, and "skipping the first" is a
> narrow special case of giving N=1.

For my workflow, it is not about "initial skip", but rather just "skip
emails that don't have patches in them at all". My MUA makes it easy to
tag a whole thread (or subthread), cover letter and discussion included,
and then dump it all to git-am.

And I think that would be the same for a public-inbox workflow (if it
learns to grab sub-threads; otherwise you end up with earlier iterations
of the series attached to the same thread).

That is solving a different problem than you, though, where you want to
skip actual patches because you know they are unchanged.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] help: make option --help open man pages only for Git commands

[Junio C Hamano]

Ralf Thielow  writes:

>  builtin/help.c  | 30 +++---
>  git.c   | 15 ++-
>  t/t0012-help.sh | 15 +++
>  3 files changed, 52 insertions(+), 8 deletions(-)
>  create mode 100755 t/t0012-help.sh
>
> diff --git a/builtin/help.c b/builtin/help.c
> index 8848013..76f07c7 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -37,7 +37,9 @@ static int show_all = 0;
>  static int show_guides = 0;
>  static unsigned int colopts;
>  static enum help_format help_format = HELP_FORMAT_NONE;
> +static int swapped = 0;

This is not the first offender (show_guides above does so, too), but
please do not initialize static explicitly to 0 or NULL.

>  static struct option builtin_help_options[] = {
> + OPT_BOOL('s', "swapped", , "mark as being called by  
> --help"),
>   OPT_BOOL('a', "all", _all, N_("print all available commands")),
>   OPT_BOOL('g', "guides", _guides, N_("print list of useful 
> guides")),
>   OPT_SET_INT('m', "man", _format, N_("show man page"), 
> HELP_FORMAT_MAN),
> @@ -433,10 +435,29 @@ static void list_common_guides_help(void)
>   putchar('\n');
>  }
>  
> +static const char* check_git_cmd(const char* cmd)

Style: "static const char *check_git_cmd(const char *cmd)".  The
asterisk that turns the base type to a pointer to the base type
sticks to the identifier, not to the type.

> +{
> + char *alias;
> +
> + if (is_git_command(cmd))
> + return cmd;
> +
> + alias = alias_lookup(cmd);
> + if (alias) {
> + printf_ln(_("`git %s' is aliased to `%s'"), cmd, alias);
> + free(alias);
> + exit(0);
> + }
> +
> + if (swapped)
> + return help_unknown_cmd(cmd);

I am guilty of suggesting "swapped"; even if we are going to mark
this as OPT_HIDDEN, I think we should be able to think of a better
name.  I think the meaning of this boolean is "we know that this is
not a guide and is meant to be a command.", and I hope we can come
up with a name that concisely expresses that (e.g. "--not-a-guide",
"--must-be-a-command").

> + return cmd;
> +}
> +
>  int cmd_help(int argc, const char **argv, const char *prefix)
>  {
>   int nongit;
> - char *alias;
>   enum help_format parsed_help_format;
>  
>   argc = parse_options(argc, argv, prefix, builtin_help_options,
> @@ -476,12 +497,7 @@ int cmd_help(int argc, const char **argv, const char 
> *prefix)
>   if (help_format == HELP_FORMAT_NONE)
>   help_format = parse_help_format(DEFAULT_HELP_FORMAT);
>  
> - alias = alias_lookup(argv[0]);
> - if (alias && !is_git_command(argv[0])) {
> - printf_ln(_("`git %s' is aliased to `%s'"), argv[0], alias);
> - free(alias);
> - return 0;
> - }
> + argv[0] = check_git_cmd(argv[0]);
>  
>   switch (help_format) {
>   case HELP_FORMAT_NONE:
> diff --git a/git.c b/git.c
> index 0f1937f..71ea983 100644
> --- a/git.c
> +++ b/git.c
> @@ -528,10 +528,23 @@ static void handle_builtin(int argc, const char **argv)
>   strip_extension(argv);
>   cmd = argv[0];
>  
> - /* Turn "git cmd --help" into "git help cmd" */
> + /* Turn "git cmd --help" into "git help --swapped cmd" */
>   if (argc > 1 && !strcmp(argv[1], "--help")) {
> + struct argv_array args;
> + int i;
> +
>   argv[1] = argv[0];
>   argv[0] = cmd = "help";
> +
> + argv_array_init();
> + for (i = 0; i < argc; i++) {
> + argv_array_push(, argv[i]);
> + if (i == 0)

It is more idiomatic to say

if (!i)

around here.

> + argv_array_push(, "--swapped");

> + }
> +
> + argc++;
> + argv = argv_array_detach();
>   }
>  
>   builtin = get_builtin(cmd);

The code does this after it:

if (builtin)
exit(run_builtin(...));

and returns.  If we didn't get builtin, we risk leaking args.argv
here, but we assume argv[0] = cmd = "help" is always a builtin,
which I think is a safe assumption, so the code is OK.  Static
checkers that are only half intelligent may yell at you for not
releasing the resources, though.

> diff --git a/t/t0012-help.sh b/t/t0012-help.sh
> new file mode 100755
> index 000..6f700b1
> --- /dev/null
> +++ b/t/t0012-help.sh
> @@ -0,0 +1,15 @@
> +#!/bin/sh
> +
> +test_description='help'
> +
> +. ./test-lib.sh
> +
> +test_expect_success "pass --help to common guide" "
> + cat <<-EOF >expected &&
> + git: 'revisions' is not a git command. See 'git --help'.
> + EOF
> + (git revisions --help 2>actual || true) &&
> + test_i18ncmp expected actual
> +"
> +
> +test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  

Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]

[Junio C Hamano]

Stefan Beller  writes:

> So as a discussion starter:
> * Should git am skip a patch 00/XX automatically ?

No.  My preference is to add "--initial-skip=", though.

When I receive a patch series to reroll another series, I somehow
know and verify that earlier N patches have not changed, I detach
the HEAD at the last unchanged commit from the previous round and
apply the remainder of the new series, so that I can preserve the
author timestamps of earlier steps from the previous series.  By
the time I "know and verify" where the first step that was updated,
I have a full series in a single mbox; having "--initial-skip="
would help with that use case, too, and "skipping the first" is a
narrow special case of giving N=1.

> * Should the public-inbox offer another link to patches 1-n, without
>   the cover letter? Or should it add instructions:
>
> If this is a patch series you can apply it locally as:
> curl  >tmpXXX
> git am tmpXXX && git am --skip && git am --continue

I do not think it is sensible for "cover-letter" specific
instructions.  However, I do not think it is unreasonable to either
add another mbox.gz link or replace the behaviour of mbox.gz link so
that you can grab a mbox that contains "this message and everything
after it in the thread".  That way, I could open the first message,
see something like this I found in your message:

>> Thread overview: 4+ messages in thread (expand / mbox.gz / Atom feed / [top])
>> 2016-08-15 23:06 Jacob Keller [this message]
>> 2016-08-15 23:07 ` [PATCH v6 1/3] diff.c: remove output_prefix_length field 
>> Jacob Keller
>> 2016-08-15 23:07 ` [PATCH v6 2/3] graph: add support for --line-prefix on 
>> all graph-aware output Jacob Keller
>> 2016-08-15 23:07 ` [PATCH v6 3/3] diff: add SUBMODULE_DIFF format to display 
>> submodule diff Jacob Keller

and then go to 1/3 and click that "this and everything that
follows".
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Working with zip files

[Junio C Hamano]

David Lang  writes:

> you should be able to use clean/smudge to have git store the files
> uncompressed, which will help a lot.
>
> I think there's a way to tell it to do a xml aware diff/patch, but I
> don't remember how.

I do not know about "patch" (in the sense of "git apply"), but "git
diff" (and "git log -p") can take advantage of the clean/smudge
mechanism.  I used to deal with a file format that is gzipped xml so
my clean filter was "gzip -dc" while the smudge was "gzip -cn".
Essentially, this stors the xml before compression in the repository
so blobs delta well with each other and also the revisions are
made textually diff-able.

Nikolaus's case has one extra layer of complexity in that the "file"
is actually an archive of multiple files.  The clean/smudge pair he
writes need to be a filter that flattens the archive into a single
human-readable text byte stream and its reverse.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]

[Stefan Beller]

> BTW in light of the discussion we are having elsewhere I just need to
> point out that it was *dramatically* faster for me to edit run-command.c,
> find "hooks/" and adjust the code manually than it would have been to save
> the diff and apply it.
>
> That's because I do not have advanced tooling on top of email (and I also
> could not handle mutt, so I am stuck with a not-really-scriptable email
> client).
>
> Just sayin'.

I ran into the same problem, just for a larger patch, so I figured I can
download that from the public inbox and git-am it locally.
So I maneuvered to the cover letter of the patch series I am interested in[1]
and downloaded the series as a mbox.gz[2].

However git-am choked on the cover-letter with:
> Patch is empty. Was it split wrong?
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".

The way forward from here was to `git am --skip` the first mail (the
cover letter)
and the rest got applied cleanly.

So as a discussion starter:
* Should git am skip a patch 00/XX automatically ?
  It is obviously a cover letter, which may be text only or
  has an intra diff to a prior version. Neither is what we want for now.
  Although there is this other discussion of storing the cover letter,
  so maybe an empty patch that is numbered 0 is fine to skip for now?
  Once the discussion settles whether we want to store it in
  branch..description or as an empty commit at the end or at the beginning

* Should the public-inbox offer another link to patches 1-n, without
  the cover letter? Or should it add instructions:

If this is a patch series you can apply it locally as:
curl  >tmpXXX
git am tmpXXX && git am --skip && git am --continue

I tend to favor the first option of Git learning how to process the
cover letter more easily.

Thanks,
Stefan


[1] 
https://public-inbox.org/git/20160815230702.30817-1-jacob.e.kel...@intel.com/
[2] 
https://public-inbox.org/git/20160815230702.30817-1-jacob.e.kel...@intel.com/t.mbox.gz
as found in
> Thread overview: 4+ messages in thread (expand / mbox.gz / Atom feed / [top])
> 2016-08-15 23:06 Jacob Keller [this message]
> 2016-08-15 23:07 ` [PATCH v6 1/3] diff.c: remove output_prefix_length field 
> Jacob Keller
> 2016-08-15 23:07 ` [PATCH v6 2/3] graph: add support for --line-prefix on all 
> graph-aware output Jacob Keller
> 2016-08-15 23:07 ` [PATCH v6 3/3] diff: add SUBMODULE_DIFF format to display 
> submodule diff Jacob Keller
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Working with zip files

[David Lang]

On Tue, 16 Aug 2016, Nikolaus Rath wrote:


On Aug 16 2016, David Lang  wrote:

On Tue, 16 Aug 2016, Nikolaus Rath wrote:


I would like to store Simulink models in a Git
repository. Unfortunately, the file format is binary. But luckily, the
binary format happens to be a zipfile containing nicely formatted XML
files.

Is there a way to teach Git to take advantage of this when storing,
diff-ing and merging these files?


you should be able to use clean/smudge to have git store the files
uncompressed, which will help a lot.


Cool, I'll look into that.


I think there's a way to tell it to do a xml aware diff/patch, but I
don't remember how.


Oh, I didn't even want to go that far. I'm perfectly happy if it does a
text-based diff/patch of the contained XML files. Would clean/smudge
provide that already?


yes.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Working with zip files

[Nikolaus Rath]

On Aug 16 2016, David Lang  wrote:
> On Tue, 16 Aug 2016, Nikolaus Rath wrote:
>
>> I would like to store Simulink models in a Git
>> repository. Unfortunately, the file format is binary. But luckily, the
>> binary format happens to be a zipfile containing nicely formatted XML
>> files.
>>
>> Is there a way to teach Git to take advantage of this when storing,
>> diff-ing and merging these files?
>
> you should be able to use clean/smudge to have git store the files
> uncompressed, which will help a lot.

Cool, I'll look into that.

> I think there's a way to tell it to do a xml aware diff/patch, but I
> don't remember how.

Oh, I didn't even want to go that far. I'm perfectly happy if it does a
text-based diff/patch of the contained XML files. Would clean/smudge
provide that already?


Best,
-Nikolaus

-- 
GPG encrypted emails preferred. Key id: 0xD113FCAC3C4E599F
Fingerprint: ED31 791B 2C5C 1613 AF38 8B8A D113 FCAC 3C4E 599F

 »Time flies like an arrow, fruit flies like a Banana.«
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] help: make option --help open man pages only for Git commands

[Ralf Thielow]

2016-08-16 18:33 GMT+02:00 John Keeping :
> On Tue, Aug 16, 2016 at 06:20:30PM +0200, Ralf Thielow wrote:
>>  static struct option builtin_help_options[] = {
>> + OPT_BOOL('s', "swapped", , "mark as being called by  
>> --help"),
>
> OPT_HIDDEN_BOOL maybe?
>

Yeah >_<

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: upload-pack/ls-remote: showing non-HEAD symbolic refs?

[Jeff King]

On Tue, Aug 16, 2016 at 09:18:39AM -0700, Josh Triplett wrote:

> Commit 5e7dcad771cb873e278a0571b46910d7c32e2f6c in September 2013 added
> support to upload-pack to show the symbolic target of non-HEAD symbolic
> refs.  However, commit d007dbf7d6d647dbcf0f357545f43f36dec46f3b in
> November 2013 reverted that, because it used a capability to transmit
> the information, and capabilities have a limited size (limited by the
> pkt-line format which can't send lines longer than 64k) and can't
> transmit an arbitrary number of symrefs.
> 
> (Incidentally, couldn't the same problem occur if the HEAD points to a
> long enough path to exceed 64k?

Yes. But it's a lot easier to say "your 64k symref is ridiculous; don't
do that" than it is to say "oh, you happened to have a lot of symrefs in
your repository, so we overflowed and failed".

Besides which, the whole protocol cannot handle refnames larger than
64k, so it's not a new problem.

> I'd like to be able to see the targets of non-HEAD symbolic refs for a
> repository (symbolic refs under refs/).  I'm interested in extending
> upload-pack to expose those somehow.  What seems like a sensible format
> to do so?
> 
> Would it make sense to advertise a new capability for symbolic ref
> targets, which would allow the client to send back a dedicated request
> for the targets of all symrefs?

It will definitely require a new capability. You cannot just send a
"\0symref=..." trailer after each ref, because older clients treat
multiple "\0" trailers as overwriting one another (so it essentially
overwrites the old capabilities).

Sadly you cannot use a capability to fix that, because all of this
happens before the client agrees to any capabilities (you can find
discussion of a "v2" protocol on the list which solves this, but it's
sort of languishing in the design phase).

So you are stuck introducing a new phase into the protocol, which is
probably rather tricky (especially with the http protocol, which is very
sensitive to extra round-trips). I guess the least-invasive way would be
to communicate the desires in the "want" phase, and then have the server
dump it out with the packfile. Like:

  - server claims "I support symref-wants" in the capability phase

  - during the negotiation phase, in addition to "want" and "have", the
client may send "symref " packets. Probably  should be a
wildcard to avoid having to ask about each ref individually.

  - before outputting the packfile in the final phase, if any "symref"
wants were sent by the client, the server dumps a list of "symref
 " packets, followed by a flush packet.

That should Just Work over the existing http protocol without requiring
an extra request.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] help: make option --help open man pages only for Git commands

[John Keeping]

On Tue, Aug 16, 2016 at 06:20:30PM +0200, Ralf Thielow wrote:
> If option --help is passed to a Git command, we try to open
> the man page of that command.  However, we do it even for commands
> we don't know.  Make sure it is a Git command by using "help_unknown_cmd"
> which is even able to assume a command if the user made a typo.
> 
> This breaks "git  --help" while "git help " still works.
> 
> As " --help" will internally be turned into "help ",
> introduce the hidden option "--swapped" in order to know which
> version has been called.
> 
> Signed-off-by: Ralf Thielow 
> ---
> Thanks, all, for the help!
> 
> Changes since v2:
> - don't check for common guides as the list is very incomplete
> - only check for git commands when called via  --help (introduce
>   option --swapped for that), as suggested by Junio
> - change test case to check for --help being passed to a concept
>   used as a git command
> 
>  builtin/help.c  | 30 +++---
>  git.c   | 15 ++-
>  t/t0012-help.sh | 15 +++
>  3 files changed, 52 insertions(+), 8 deletions(-)
>  create mode 100755 t/t0012-help.sh
> 
> diff --git a/builtin/help.c b/builtin/help.c
> index 8848013..76f07c7 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -37,7 +37,9 @@ static int show_all = 0;
>  static int show_guides = 0;
>  static unsigned int colopts;
>  static enum help_format help_format = HELP_FORMAT_NONE;
> +static int swapped = 0;
>  static struct option builtin_help_options[] = {
> + OPT_BOOL('s', "swapped", , "mark as being called by  
> --help"),

OPT_HIDDEN_BOOL maybe?

>   OPT_BOOL('a', "all", _all, N_("print all available commands")),
>   OPT_BOOL('g', "guides", _guides, N_("print list of useful 
> guides")),
>   OPT_SET_INT('m', "man", _format, N_("show man page"), 
> HELP_FORMAT_MAN),
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Working with zip files

[David Lang]

On Tue, 16 Aug 2016, Nikolaus Rath wrote:


I would like to store Simulink models in a Git
repository. Unfortunately, the file format is binary. But luckily, the
binary format happens to be a zipfile containing nicely formatted XML
files.

Is there a way to teach Git to take advantage of this when storing,
diff-ing and merging these files?


you should be able to use clean/smudge to have git store the files uncompressed, 
which will help a lot.


I think there's a way to tell it to do a xml aware diff/patch, but I don't 
remember how.


David Lang
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Working with zip files

[Nikolaus Rath]

Hello,

I would like to store Simulink models in a Git
repository. Unfortunately, the file format is binary. But luckily, the
binary format happens to be a zipfile containing nicely formatted XML
files.

Is there a way to teach Git to take advantage of this when storing,
diff-ing and merging these files?

Best,
-Nikolaus

-- 
GPG encrypted emails preferred. Key id: 0xD113FCAC3C4E599F
Fingerprint: ED31 791B 2C5C 1613 AF38 8B8A D113 FCAC 3C4E 599F

 »Time flies like an arrow, fruit flies like a Banana.«
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3] help: make option --help open man pages only for Git commands

[Ralf Thielow]

If option --help is passed to a Git command, we try to open
the man page of that command.  However, we do it even for commands
we don't know.  Make sure it is a Git command by using "help_unknown_cmd"
which is even able to assume a command if the user made a typo.

This breaks "git  --help" while "git help " still works.

As " --help" will internally be turned into "help ",
introduce the hidden option "--swapped" in order to know which
version has been called.

Signed-off-by: Ralf Thielow 
---
Thanks, all, for the help!

Changes since v2:
- don't check for common guides as the list is very incomplete
- only check for git commands when called via  --help (introduce
  option --swapped for that), as suggested by Junio
- change test case to check for --help being passed to a concept
  used as a git command

 builtin/help.c  | 30 +++---
 git.c   | 15 ++-
 t/t0012-help.sh | 15 +++
 3 files changed, 52 insertions(+), 8 deletions(-)
 create mode 100755 t/t0012-help.sh

diff --git a/builtin/help.c b/builtin/help.c
index 8848013..76f07c7 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -37,7 +37,9 @@ static int show_all = 0;
 static int show_guides = 0;
 static unsigned int colopts;
 static enum help_format help_format = HELP_FORMAT_NONE;
+static int swapped = 0;
 static struct option builtin_help_options[] = {
+   OPT_BOOL('s', "swapped", , "mark as being called by  
--help"),
OPT_BOOL('a', "all", _all, N_("print all available commands")),
OPT_BOOL('g', "guides", _guides, N_("print list of useful 
guides")),
OPT_SET_INT('m', "man", _format, N_("show man page"), 
HELP_FORMAT_MAN),
@@ -433,10 +435,29 @@ static void list_common_guides_help(void)
putchar('\n');
 }
 
+static const char* check_git_cmd(const char* cmd)
+{
+   char *alias;
+
+   if (is_git_command(cmd))
+   return cmd;
+
+   alias = alias_lookup(cmd);
+   if (alias) {
+   printf_ln(_("`git %s' is aliased to `%s'"), cmd, alias);
+   free(alias);
+   exit(0);
+   }
+
+   if (swapped)
+   return help_unknown_cmd(cmd);
+
+   return cmd;
+}
+
 int cmd_help(int argc, const char **argv, const char *prefix)
 {
int nongit;
-   char *alias;
enum help_format parsed_help_format;
 
argc = parse_options(argc, argv, prefix, builtin_help_options,
@@ -476,12 +497,7 @@ int cmd_help(int argc, const char **argv, const char 
*prefix)
if (help_format == HELP_FORMAT_NONE)
help_format = parse_help_format(DEFAULT_HELP_FORMAT);
 
-   alias = alias_lookup(argv[0]);
-   if (alias && !is_git_command(argv[0])) {
-   printf_ln(_("`git %s' is aliased to `%s'"), argv[0], alias);
-   free(alias);
-   return 0;
-   }
+   argv[0] = check_git_cmd(argv[0]);
 
switch (help_format) {
case HELP_FORMAT_NONE:
diff --git a/git.c b/git.c
index 0f1937f..71ea983 100644
--- a/git.c
+++ b/git.c
@@ -528,10 +528,23 @@ static void handle_builtin(int argc, const char **argv)
strip_extension(argv);
cmd = argv[0];
 
-   /* Turn "git cmd --help" into "git help cmd" */
+   /* Turn "git cmd --help" into "git help --swapped cmd" */
if (argc > 1 && !strcmp(argv[1], "--help")) {
+   struct argv_array args;
+   int i;
+
argv[1] = argv[0];
argv[0] = cmd = "help";
+
+   argv_array_init();
+   for (i = 0; i < argc; i++) {
+   argv_array_push(, argv[i]);
+   if (i == 0)
+   argv_array_push(, "--swapped");
+   }
+
+   argc++;
+   argv = argv_array_detach();
}
 
builtin = get_builtin(cmd);
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
new file mode 100755
index 000..6f700b1
--- /dev/null
+++ b/t/t0012-help.sh
@@ -0,0 +1,15 @@
+#!/bin/sh
+
+test_description='help'
+
+. ./test-lib.sh
+
+test_expect_success "pass --help to common guide" "
+   cat <<-EOF >expected &&
+   git: 'revisions' is not a git command. See 'git --help'.
+   EOF
+   (git revisions --help 2>actual || true) &&
+   test_i18ncmp expected actual
+"
+
+test_done
-- 
2.9.2.912.g69c5047

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] receive-pack: allow a maximum input size to be specified

[Jeff King]

On Tue, Aug 16, 2016 at 04:27:10PM +0200, Christian Couder wrote:

> >> +while read unpacklimit filesize filename
> >> +do
> >> [...]
> >> +done <<\EOF
> >> +1 1024 one-k-file
> >> +10 2048 two-k-file
> >> +EOF
> >
> > Is there any reason to use different filenames and sizes for the two
> > runs? They should behave identically, so it would make more sense to me
> > to subject them to identical inputs.
> 
> About the sizes, I thought that some people might want to try sizes
> closer to the limit and also that it is good anyway in general to add
> a bit of "randomness", or at least variability, in the tests.

In general, I'd prefer a systematic approach to introducing variables
into tests. If it's important that we test different sizes, then we
should do so, and not only test some combinations (and if it is not
important to test different sizes, then we should not waste CPU time and
the mental energy of people reading the tests).

IOW, when I see this, I wonder why the index-pack code path is not
tested against a 2k file. But there really isn't a good reason. So
either it does matter, and our tests do not have good coverage, or it
does not, and it is just making me wonder if the tests are buggy.

Worse, both files are created with the same seed via test-genrandom. So
I suspect the 2k file is a superset of the 1k file, and we may send it
is a thin delta (so it really is only testing a 1k input anyway!).

> I thought that it would be a bit less wasteful to use the same "dest"
> and also it would make the test more realistic as people often push in
> non empty repos.

I doubt it is expensive enough to matter in practice. Though note that
if you push the same commit to two new repositories, then you can
amortize the test-genrandom/add/commit step (i.e., push the exact same
packfile in both cases).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

upload-pack/ls-remote: showing non-HEAD symbolic refs?

[Josh Triplett]

Commit 5e7dcad771cb873e278a0571b46910d7c32e2f6c in September 2013 added
support to upload-pack to show the symbolic target of non-HEAD symbolic
refs.  However, commit d007dbf7d6d647dbcf0f357545f43f36dec46f3b in
November 2013 reverted that, because it used a capability to transmit
the information, and capabilities have a limited size (limited by the
pkt-line format which can't send lines longer than 64k) and can't
transmit an arbitrary number of symrefs.

(Incidentally, couldn't the same problem occur if the HEAD points to a
long enough path to exceed 64k?  Unlikely to arise in practice, but
theoretically possible for a constructed repository.  Not a major
problem at the moment, since send-pack doesn't seem to support
*sending* symbolic refs, but it would become a problem given any
mechanism to send symbolic refs to the server.)

I'd like to be able to see the targets of non-HEAD symbolic refs for a
repository (symbolic refs under refs/).  I'm interested in extending
upload-pack to expose those somehow.  What seems like a sensible format
to do so?

Would it make sense to advertise a new capability for symbolic ref
targets, which would allow the client to send back a dedicated request
for the targets of all symrefs?

- Josh Triplett
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] rev-parse: respect core.hooksPath in --git-path

[Remi Galan Alfonso]

Johannes Schindelin  writes:
> Hi Rémi,
> 
> On Tue, 16 Aug 2016, Remi Galan Alfonso wrote:
> 
> > Johannes Schindelin  writes:
> > > diff --git a/t/t1350-config-hooks-path.sh b/t/t1350-config-hooks-path.sh
> > > index 5e3fb3a..f1f9aee 100755
> > > --- a/t/t1350-config-hooks-path.sh
> > > +++ b/t/t1350-config-hooks-path.sh
> > > @@ -34,4 +34,10 @@ test_expect_success 'Check that various forms of 
> > > specifying core.hooksPath work'
> > >  test_cmp expect actual
> > >  '
> > >  
> > > +test_expect_success 'git rev-parse --git-path hooks' '
> > > +git config core.hooksPath .git/custom-hooks &&
> >
> > Any reason to not use 'test_config' here?
> 
> Yes: consistency. The rest of the script uses `git config`, not
> `test_config`.

Fine by me, then. Sorry for the noise.

> Ciao,
> Dscho

Ciao,
Rémi
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Credential helpers processing order

[Jeff King]

On Tue, Aug 16, 2016 at 05:13:55PM +0200, Dmitry Neverov wrote:

> I wonder why credential helpers are called in the order: system,
> global, local, command-line and not in the reverse order? This make it
> impossible to provide a custom helper and disable default ones via
> command-line parameter. My use-case is to clone a repository in a
> non-interactive environment where a system-level GUI helper is
> configured: clone hangs since system-level helper called first and
> there is no input from the user. Also if a system-level helper sets
> quit=true, then lower-level helpers won't be called at all. Is it by
> design?

I agree that it's not really a sensible order. But reversing them isn't
quite right either. The problem is that the config code just gives the
credential code a sequence of items, with no indication which file they
came from, with what priority, etc.

For config keys with a single value, "last one wins" makes sense; we
just keep overwriting the previous value.

But for lists (like credential.helper, but also other things like
remote.*.fetch), we just build up the list. And we can't tell the
difference between two items next to each in the same file, or two in
different files with differing priorities.

Fixing that would be tricky. But I think for your case (and most similar
cases), you'd be happy to just be able to "reset" the list to empty. As
of git v2.9.0, you can do that, like so:

  [credential]
  helper = "!echo >&2 should not run;:"
  helper =
  helper = "!echo >&2 should run;:"

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] rev-parse: respect core.hooksPath in --git-path

[Johannes Schindelin]

Hi Rémi,

On Tue, 16 Aug 2016, Remi Galan Alfonso wrote:

> Johannes Schindelin  writes:
> > diff --git a/t/t1350-config-hooks-path.sh b/t/t1350-config-hooks-path.sh
> > index 5e3fb3a..f1f9aee 100755
> > --- a/t/t1350-config-hooks-path.sh
> > +++ b/t/t1350-config-hooks-path.sh
> > @@ -34,4 +34,10 @@ test_expect_success 'Check that various forms of 
> > specifying core.hooksPath work'
> >  test_cmp expect actual
> >  '
> >  
> > +test_expect_success 'git rev-parse --git-path hooks' '
> > +git config core.hooksPath .git/custom-hooks &&
> 
> Any reason to not use 'test_config' here?

Yes: consistency. The rest of the script uses `git config`, not
`test_config`.

Ciao,
Dscho

Re: Credential helpers processing order

[Jacob Keller]

On Tue, Aug 16, 2016 at 8:13 AM, Dmitry Neverov
 wrote:
> Hi,
>
> I wonder why credential helpers are called in the order: system,
> global, local, command-line and not in the reverse order? This make it
> impossible to provide a custom helper and disable default ones via
> command-line parameter. My use-case is to clone a repository in a
> non-interactive environment where a system-level GUI helper is
> configured: clone hangs since system-level helper called first and
> there is no input from the user. Also if a system-level helper sets
> quit=true, then lower-level helpers won't be called at all. Is it by
> design?
>

If I understand correctly, the credential helpers aren't supposed to
require input so it is assumed they can be tried in sequence if one
fails? It might make sense to reverse the order though...

Thanks,
Jake

> --
> Dmitry
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: storing cover letter of a patch series?

[Jacob Keller]

On Mon, Aug 15, 2016 at 11:45 PM, Duy Nguyen  wrote:
> On Tue, Aug 16, 2016 at 12:26 PM, Jacob Keller  wrote:
 They can just add "squash! cover! " commits for that ;-) Though 
 more
 likely the advanced workflow would be used... We'll need both (more than
 one) options.
>>>
>>> Or even better, "git commit --reword $SHA1" brings up the editor with
>>> commit message of $SHA1. Modify any way you want and it creates a new
>>> empty, "reword!" commit that contains the diff between the old commit
>>> message and the new one. "reword!" can be consumed by "rebase -i
>>> --autosquash" without bringing up the editor again. I realize making
>>> "git commit --reword" run multiple times would be tricky though...
>>
>> I was just thinking you write text and it gets appended to the text of
>> the reworded commit, and when you squash them using rebase you get to
>> finalize it like a normal squash?
>
> I think that's what Phillip meant by 'squash! cover!' though I wanted
> to go further, I don't want an editor popping up at rebase time,
> instead 'rebase' just update cover letter automatically for me.
> --
> Duy

Maybe teach it some sort of "reword! cover!" which pops up an editor
and you can edit to your hearts content, and it just saves the "new"
message. Since there is no such thing as a diff on message contents,
it would just be a complete replace for the new message (obviously we
would then strip reword and cover part out but otherwise leave the
rest in place so rebase machinery would be able to fix it up without
you having to edit it a second time in the rebase process? That
doesn't seem as complicated as somehow storing a new diff format for
the cover letter. Not sure how to handle several in a row though.

Thanks,
Jake
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] squash! diff: add --diff-line-prefix option for passing in a prefix

[Johannes Schindelin]

Hi Junio,

On Mon, 15 Aug 2016, Junio C Hamano wrote:

> Obviously (1) is a lot of impact with little gain, and as Jacob
> already offered to do, I think (2) is a lot more sensible solution
> and it also is more in line with your "If it isn't broken, do not
> fix it", I would say.

Yep, that makes most sense to me, too.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] t/Makefile: make sure that file names are truly platform-independent

[Jeff King]

On Tue, Aug 16, 2016 at 05:37:35PM +0200, Johannes Schindelin wrote:

> > Hrm. I am not sure I agree. At GitHub, for instance, we turn on
> > core.protectNTFS for all repositories because we do want to be a vector
> > for attacks.
> 
> I trust you meant "do *not* want to be a vector for attacks"...

Heh, yes.

> Good point.
> 
> What I meant in my curt language was actually not to use core.protectNTFS
> per se, but the same code path. That is, I would rather have any such
> "cross-platform helping" code in verify_path() rather than
> write_index_as_tree().
> 
> But you are correct, this hypothetical feature (pretty hypothetical,
> indeed, at this point) would have to be configured differently than
> via core.protectNTFS=true.

Ah, OK. Yeah, I don't have any real objection to that. I'm not sure how
easy it would be to check verify_path() for an existing commit (say, as
part of a pre-commit hook, or a pre-receive, or in a CI test). But if it
is not easy, that seems like a problem that can (and should) be fixed
separately.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] t/Makefile: make sure that file names are truly platform-independent

[Johannes Schindelin]

Hi Peff,

On Tue, 16 Aug 2016, Jeff King wrote:

> On Tue, Aug 16, 2016 at 03:10:46PM +0200, Johannes Schindelin wrote:
> 
> > > I am not convinced this mechanism needs to be built into git.
> > > Because it happens to be about filenames, git at least has a hope of
> > > making sense of the various project rules.
> > 
> > Both of you gentle people may recall a conversation in December 2014
> > when we scrambled to plug a hole where maliciously-chosen file names
> > would have allowed to wreak havoc with a local Git repository's config
> > (among other things).
> > 
> > We did plug it, but not before I proposed to exclude many more file
> > names than just maliciously-chosen ones. For example, I wanted to
> > exclude all file names that are illegal on Windows when
> > core.protectNTFS was set to true.
> > 
> > If we were to implement this "let's help cross-platform projects"
> > functionality, it would be at that same level.
> 
> Hrm. I am not sure I agree. At GitHub, for instance, we turn on
> core.protectNTFS for all repositories because we do want to be a vector
> for attacks.

I trust you meant "do *not* want to be a vector for attacks"...

> So the tradeoff is a good one: the restrictions on filenames are not
> that big, and we gain a lot of safety (i.e., a known remote code
> execution bug).
> 
> Whereas if core.protectNTFS started disallowing trees with both "foo"
> and "FOO", that is a much different tradeoff. It is much more likely to
> come up, and it is protecting a much less valuable thing (it's an
> annoyance, not a security hole). Projects which do not care about people
> on case-insensitive filesystems will be annoyed to have their commits
> rejected (whether they are right to be so uncaring or not can be
> debated, but I am not sure that GitHub wants to enforce a hard policy at
> the fsck layer).
> 
> So even if we wanted a similar mechanism, I think it has to be triggered
> by a separate config option. And I do not think general hosting sites
> would turn it on. It's really a project decision, not a hosting-site
> one.
> 
> There may be some rules that are in between. I.e., names that are
> illegal on some common platform but are extremely unlikely to be chosen
> in general. I'd have to see the rules to give an opinion.

Good point.

What I meant in my curt language was actually not to use core.protectNTFS
per se, but the same code path. That is, I would rather have any such
"cross-platform helping" code in verify_path() rather than
write_index_as_tree().

But you are correct, this hypothetical feature (pretty hypothetical,
indeed, at this point) would have to be configured differently than
via core.protectNTFS=true.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Credential helpers processing order

[Dmitry Neverov]

Hi,

I wonder why credential helpers are called in the order: system,
global, local, command-line and not in the reverse order? This make it
impossible to provide a custom helper and disable default ones via
command-line parameter. My use-case is to clone a repository in a
non-interactive environment where a system-level GUI helper is
configured: clone hangs since system-level helper called first and
there is no input from the user. Also if a system-level helper sets
quit=true, then lower-level helpers won't be called at all. Is it by
design?

--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3] t/Makefile: ensure that paths are valid on platforms we care

[Johannes Schindelin]

Some pathnames that are okay on ext4 and on HFS+ cannot be checked
out on Windows. Tests that want to see operations on such paths on
filesystems that support them must do so behind appropriate test
prerequisites, and must not include them in the source tree (instead
they should create them when they run). Otherwise, the source tree
cannot even be checked out.

Make sure that double-quotes, asterisk, colon, greater/less-than,
question-mark, backslash, tab, vertical-bar, as well as any non-ASCII
characters never appear in the pathnames with a new test-lint-* target
as part of a `make test`. To that end, we call `git ls-files` (ensuring
that the paths are quoted properly), relying on the fact that paths
containing non-ASCII characters are quoted within double-quotes.

In case that the source code does not actually live in a Git
repository (e.g. when extracted from a .zip file), or that the `git`
executable cannot be executed, we simply ignore the error for now; In
that case, our trusty Continuous Integration will be the last line of
defense and catch any problematic file name.

Noticed when a topic wanted to add a pathname with '>' in it.  A
check like this will prevent a similar problems from happening in the
future.

Signed-off-by: Johannes Schindelin 
---

Again some touch-ups to the commit message.

Please note that the paths are now quoted within double-quotes,
thanks to the core.quotepaths setting. As such, non-ASCII
characters are no longer caught through the backslash, but through
the double-quote character (i.e. '"').

As previously, tested on Linux and Windows and verified that it
does the job.

Published-As: https://github.com/dscho/git/releases/tag/test-lint-filenames-v3
Fetch-It-Via: git fetch https://github.com/dscho/git test-lint-filenames-v3
Interdiff vs v2:

 diff --git a/t/Makefile b/t/Makefile
 index bf9cad9..d613935 100644
 --- a/t/Makefile
 +++ b/t/Makefile
 @@ -69,9 +69,12 @@ test-lint-shell-syntax:
@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS)
  
  test-lint-filenames:
 -  @illegal="$$(git ls-files | grep '[ "*:<>?\\|]')"; \
 -  test -z "$$illegal" || { \
 -  echo >&2 "illegal file name(s): " $$illegal; exit 1; }
 +  @# We do *not* pass a glob to ls-files but use grep instead, to catch
 +  @# non-ASCII characters (which are quoted within double-quotes)
 +  @bad="$$(git -c core.quotepath=true ls-files 2>/dev/null | \
 +  grep '["*:<>?\\|]')"; \
 +  test -z "$$bad" || { \
 +  echo >&2 "non-portable file name(s): $$bad"; exit 1; }
  
  aggregate-results-and-cleanup: $(T)
$(MAKE) aggregate-results


 t/Makefile | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/t/Makefile b/t/Makefile
index 18e2b28..d613935 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -52,7 +52,8 @@ clean-except-prove-cache:
 clean: clean-except-prove-cache
$(RM) .prove
 
-test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax
+test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \
+   test-lint-filenames
 
 test-lint-duplicates:
@dups=`echo $(T) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d` && \
@@ -67,6 +68,14 @@ test-lint-executable:
 test-lint-shell-syntax:
@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS)
 
+test-lint-filenames:
+   @# We do *not* pass a glob to ls-files but use grep instead, to catch
+   @# non-ASCII characters (which are quoted within double-quotes)
+   @bad="$$(git -c core.quotepath=true ls-files 2>/dev/null | \
+   grep '["*:<>?\\|]')"; \
+   test -z "$$bad" || { \
+   echo >&2 "non-portable file name(s): $$bad"; exit 1; }
+
 aggregate-results-and-cleanup: $(T)
$(MAKE) aggregate-results
$(MAKE) clean
-- 
2.9.2.691.g78954f3

base-commit: 70dce4aee55ad8a39a53f86f37a4bd400e0cac7d
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] t/Makefile: make sure that file names are truly platform-independent

[Johannes Schindelin]

Hi Junio,

On Tue, 16 Aug 2016, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> > The trick to catch non-ascii depends on core.quotepath set to true
> > (which is the default).  You would need to run ls-files with an
> > explicit "-c core.quotepath=false" to be safe.
> > ...
> > The invocation of "git -c core.quotepath=false ls-files" needs to at
> > least have 2>/dev/null to squelch an unnecessary error message.  In
> > such a scenario, we may miss a new offending pathname, but we will
> > not have false positive and I think that is the best we can do.
> >
> > Thanks.
> 
> Needless to say (1) both "false" should have been "true"; (2) I
> shouldn't be crawling out of bed and typing with hazy brain early in
> the morning.
> 
> Sorry for typoes and thinkos.

Sorry for not even expecting an answer from you that early. I will send
out a v3 in a moment that should address your concerns.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] rev-parse: respect core.hooksPath in --git-path

[Remi Galan Alfonso]

Hi Johannes,

Johannes Schindelin  writes:
> diff --git a/t/t1350-config-hooks-path.sh b/t/t1350-config-hooks-path.sh
> index 5e3fb3a..f1f9aee 100755
> --- a/t/t1350-config-hooks-path.sh
> +++ b/t/t1350-config-hooks-path.sh
> @@ -34,4 +34,10 @@ test_expect_success 'Check that various forms of 
> specifying core.hooksPath work'
>  test_cmp expect actual
>  '
>  
> +test_expect_success 'git rev-parse --git-path hooks' '
> +git config core.hooksPath .git/custom-hooks &&

Any reason to not use 'test_config' here?

Thanks,
Rémi
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] t/Makefile: make sure that file names are truly platform-independent

[Jeff King]

On Tue, Aug 16, 2016 at 03:10:46PM +0200, Johannes Schindelin wrote:

> > I am not convinced this mechanism needs to be built into git. Because it
> > happens to be about filenames, git at least has a hope of making sense
> > of the various project rules.
> 
> Both of you gentle people may recall a conversation in December 2014 when
> we scrambled to plug a hole where maliciously-chosen file names would have
> allowed to wreak havoc with a local Git repository's config (among other
> things).
> 
> We did plug it, but not before I proposed to exclude many more file names
> than just maliciously-chosen ones. For example, I wanted to exclude all
> file names that are illegal on Windows when core.protectNTFS was set to
> true.
> 
> If we were to implement this "let's help cross-platform projects"
> functionality, it would be at that same level.

Hrm. I am not sure I agree. At GitHub, for instance, we turn on
core.protectNTFS for all repositories because we do want to be a vector
for attacks. So the tradeoff is a good one: the restrictions on
filenames are not that big, and we gain a lot of safety (i.e., a known
remote code execution bug).

Whereas if core.protectNTFS started disallowing trees with both "foo"
and "FOO", that is a much different tradeoff. It is much more likely to
come up, and it is protecting a much less valuable thing (it's an
annoyance, not a security hole). Projects which do not care about people
on case-insensitive filesystems will be annoyed to have their commits
rejected (whether they are right to be so uncaring or not can be
debated, but I am not sure that GitHub wants to enforce a hard policy at
the fsck layer).

So even if we wanted a similar mechanism, I think it has to be triggered
by a separate config option. And I do not think general hosting sites
would turn it on. It's really a project decision, not a hosting-site
one.

There may be some rules that are in between. I.e., names that are
illegal on some common platform but are extremely unlikely to be chosen
in general. I'd have to see the rules to give an opinion.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] limit the size of the packs we receive

[Jeff King]

On Tue, Aug 16, 2016 at 04:44:01PM +0200, Christian Couder wrote:

>> [sizes and signedness of off_t]
> I can add something along your explanations in the commit message if
> it is prefered.

I think it's probably OK without it.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH 3/3] receive-pack: allow a maximum input size to be specified

[Jeff King]

On Tue, Aug 16, 2016 at 10:25:44AM +0200, Christian Couder wrote:

> >> > Those are patches I plan to share upstream but just haven't gotten
> >> > around to yet.
> 
> I would be happy to help if I can on these patches too!

Thanks. I'll try to extract the quarantine patches, though I have a few
other things in my backlog first. I wrote them with the intent of
upstreaming, so I think they should be fairly clean.

> [register_tempfile on odb tmpfiles]
> > Also a patch I may try to polish and share in the future, but not as
> > high priority as some of the other stuff.
> 
> I can help polishing this patch if you want.

The original patch is not worth looking at; it predated all of the
tempfile.c infrastructure, so it added its own hacky versions those
functions.

This is basically what we're doing now (this is extracted from a larger
diff, so the line numbers may be a little funny):

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 3431de2..9c34353 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -304,8 +346,10 @@ static const char *open_pack_file(const char *pack_name)
input_fd = 0;
if (!pack_name) {
static char tmp_file[PATH_MAX];
+   struct tempfile *t = xcalloc(1, sizeof(*t));
output_fd = odb_mkstemp(tmp_file, sizeof(tmp_file),
"pack/tmp_pack_XX");
+   register_tempfile(t, tmp_file);
pack_name = xstrdup(tmp_file);
} else
output_fd = open(pack_name, O_CREAT|O_EXCL|O_RDWR, 
0600);
diff --git a/pack-write.c b/pack-write.c
index 33293ce..14597b4 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "pack.h"
 #include "csum-file.h"
+#include "tempfile.h"
 
 void reset_pack_idx_option(struct pack_idx_option *opts)
 {
@@ -73,7 +74,9 @@ const char *write_idx_file(const char *index_name, struct 
pack_idx_entry **objec
} else {
if (!index_name) {
static char tmp_file[PATH_MAX];
+   struct tempfile *t = xcalloc(1, sizeof(*t));
fd = odb_mkstemp(tmp_file, sizeof(tmp_file), 
"pack/tmp_idx_XX");
+   register_tempfile(t, tmp_file);
index_name = xstrdup(tmp_file);
} else {
unlink(index_name);
@@ -327,10 +330,13 @@ int encode_in_pack_object_header(enum object_type type, 
uintmax_t size, unsigned
 
 struct sha1file *create_tmp_packfile(char **pack_tmp_name)
 {
+   struct tempfile *t = xcalloc(1, sizeof(*t));
char tmpname[PATH_MAX];
int fd;
 
fd = odb_mkstemp(tmpname, sizeof(tmpname), "pack/tmp_pack_XX");
+
+   register_tempfile(t, tmpname);
*pack_tmp_name = xstrdup(tmpname);
return sha1fd(fd, *pack_tmp_name);
 }

But it makes me wonder if we should just be switching odb_mkstemp() to
calling register_tempfile(). Or better yet, switching out
git_mkstemp_mode() for one of the tempfile.h functions (mks_tempfile_m,
I think). There may be some trickery with the allocation of "struct
tempfile" though (the struct needs to persist for the remainder of the
program, but we would not want to allocate and leak one per object, so
we need some way of reusing them).

I also wonder if it should be configurable whether to keep half-written
temporary files. In the early days, it was a good source of debugging
(e.g., look at this half-written pack and see why index-pack choked on
it). But these days I doubt that is really that helpful. So maybe it
would be OK to just drop them unconditionally.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] limit the size of the packs we receive

[Christian Couder]

On Tue, Aug 16, 2016 at 3:11 PM, Jeff King  wrote:
> On Tue, Aug 16, 2016 at 10:16:58AM +0200, Christian Couder wrote:
>
>>   - changed strtoul() to strtoumax() in the first 2 patches, as
>> suggested by Peff,
>>
>>   - changed git_config_ulong() to git_config_int64() and used PRIuMAX
>> and uintmax_t in the last patch, as suggested by Peff,
>
> Thinking a bit, off_t is actually signed. So maybe PRIdMAX (which we
> don't seem to have compat macros for) would make more sense. I dunno if
> anybody actually cares. This value shouldn't be signed anyway, and
> nobody should be approaching the limits of a 64-bit number anyway (there
> is no point in limiting the incoming pack to the exabyte range).
>
> So I'm inclined not to worry about it.

Yeah, you previously wrote:

> We seem to use strtoumax() elsewhere, so that's probably a good match
> (technically it can overflow an off_t, but in practice this value comes from
> the admin and they will set something sane).

and I thought that the same would apply to using PRIuMAX and uintmax_t
in this patch.

I can add something along your explanations in the commit message if
it is prefered.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] receive-pack: allow a maximum input size to be specified

[Christian Couder]

On Tue, Aug 16, 2016 at 3:16 PM, Jeff King  wrote:
> On Tue, Aug 16, 2016 at 10:17:01AM +0200, Christian Couder wrote:
>>
>> Cleaning up what has already been written to disk is a
>> related problem that is not addressed by this patch.
>>
>> The problem is that git-gc can clean up tmp_pack_* files
>> after their grace time expired, but that may not be
>> enough if someone tries to "git push" in a loop.
>>
>> A simple fix is to call register_tempfile() in
>> open_pack_file(), and just have index-pack clean up the
>> file on its way out.
>>
>> But there are harder cases. For instance, if somebody
>> pushes a 500MB file, and there is a pre-receive hook that
>> says "too big; I won't accept this". And then they push in
>> a loop, the incoming pack has already been accepted into
>> the repository by the time the pre-receive hook runs.
>> It's not possible to just delete it, because we don't know
>> if other simultaneous processes have started to depend on
>> the objects.
>
> IMHO, everything after "not addressed by this patch" doesn't really add
> anything. This commit has value on its own, and the discussion about the
> next steps is already documented on the list (and hopefully will be
> fixed with other patches!).

Ok, I will remove that in the next iteration.

>> +test_expect_success 'create remote repository' '
>> + git init --bare dest
>> +'
>> +
>> +# Let's run tests with different unpack limits: 1 and 10
>> +# When the limit is 1, `git receive-pack` will call `git index-pack`.
>> +# When the limit is 10, `git receive-pack` will call `git unpack-objects`.
>> +
>> +while read unpacklimit filesize filename
>> +do
>> [...]
>> +done <<\EOF
>> +1 1024 one-k-file
>> +10 2048 two-k-file
>> +EOF
>
> Is there any reason to use different filenames and sizes for the two
> runs? They should behave identically, so it would make more sense to me
> to subject them to identical inputs.

About the sizes, I thought that some people might want to try sizes
closer to the limit and also that it is good anyway in general to add
a bit of "randomness", or at least variability, in the tests.

> I know you need different files and filenames to continue pushing into
> the same "dest", but the problem is easily solved by bumping the "git
> init" into the loop.

I thought that it would be a bit less wasteful to use the same "dest"
and also it would make the test more realistic as people often push in
non empty repos.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] receive-pack: allow a maximum input size to be specified

[Jeff King]

On Tue, Aug 16, 2016 at 10:17:01AM +0200, Christian Couder wrote:

> From: Jeff King 
> 
> Receive-pack feeds its input to either index-pack or
> unpack-objects, which will happily accept as many bytes as
> a sender is willing to provide. Let's allow an arbitrary
> cutoff point where we will stop writing bytes to disk.
> 
> Cleaning up what has already been written to disk is a
> related problem that is not addressed by this patch.
> 
> The problem is that git-gc can clean up tmp_pack_* files
> after their grace time expired, but that may not be
> enough if someone tries to "git push" in a loop.
> 
> A simple fix is to call register_tempfile() in
> open_pack_file(), and just have index-pack clean up the
> file on its way out.
> 
> But there are harder cases. For instance, if somebody
> pushes a 500MB file, and there is a pre-receive hook that
> says "too big; I won't accept this". And then they push in
> a loop, the incoming pack has already been accepted into
> the repository by the time the pre-receive hook runs.
> It's not possible to just delete it, because we don't know
> if other simultaneous processes have started to depend on
> the objects.

IMHO, everything after "not addressed by this patch" doesn't really add
anything. This commit has value on its own, and the discussion about the
next steps is already documented on the list (and hopefully will be
fixed with other patches!).

> +test_expect_success 'create remote repository' '
> + git init --bare dest
> +'
> +
> +# Let's run tests with different unpack limits: 1 and 10
> +# When the limit is 1, `git receive-pack` will call `git index-pack`.
> +# When the limit is 10, `git receive-pack` will call `git unpack-objects`.
> +
> +while read unpacklimit filesize filename
> +do
> [...]
> +done <<\EOF
> +1 1024 one-k-file
> +10 2048 two-k-file
> +EOF

Is there any reason to use different filenames and sizes for the two
runs? They should behave identically, so it would make more sense to me
to subject them to identical inputs.

I know you need different files and filenames to continue pushing into
the same "dest", but the problem is easily solved by bumping the "git
init" into the loop.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] rev-parse: respect core.hooksPath in --git-path

[Johannes Schindelin]

The idea of the --git-path option is not only to avoid having to
prefix paths with the output of --git-dir all the time, but also to
respect overrides for specific common paths inside the .git directory
(e.g. `git rev-parse --git-path objects` will report the value of the
environment variable GIT_OBJECT_DIRECTORY, if set).

When introducing the core.hooksPath setting, we forgot to adjust
git_path() accordingly. This patch fixes that.

While at it, revert the special-casing of core.hooksPath in
run-command.c, as it is now no longer needed.

Signed-off-by: Johannes Schindelin 
---
Published-As: https://github.com/dscho/git/releases/tag/git-path-hooks-v2
Fetch-It-Via: git fetch https://github.com/dscho/git git-path-hooks-v2
Interdiff vs v1:

 diff --git a/run-command.c b/run-command.c
 index 33bc63a..5a4dbb6 100644
 --- a/run-command.c
 +++ b/run-command.c
 @@ -824,10 +824,7 @@ const char *find_hook(const char *name)
static struct strbuf path = STRBUF_INIT;
  
strbuf_reset();
 -  if (git_hooks_path)
 -  strbuf_addf(, "%s/%s", git_hooks_path, name);
 -  else
 -  strbuf_git_path(, "hooks/%s", name);
 +  strbuf_git_path(, "hooks/%s", name);
if (access(path.buf, X_OK) < 0)
return NULL;
return path.buf;


 path.c   | 2 ++
 run-command.c| 5 +
 t/t1350-config-hooks-path.sh | 6 ++
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/path.c b/path.c
index 17551c4..fe3c4d9 100644
--- a/path.c
+++ b/path.c
@@ -380,6 +380,8 @@ static void adjust_git_path(struct strbuf *buf, int 
git_dir_len)
  get_index_file(), strlen(get_index_file()));
else if (git_db_env && dir_prefix(base, "objects"))
replace_dir(buf, git_dir_len + 7, get_object_directory());
+   else if (git_hooks_path && dir_prefix(base, "hooks"))
+   replace_dir(buf, git_dir_len + 5, git_hooks_path);
else if (git_common_dir_env)
update_common_dir(buf, git_dir_len, NULL);
 }
diff --git a/run-command.c b/run-command.c
index 33bc63a..5a4dbb6 100644
--- a/run-command.c
+++ b/run-command.c
@@ -824,10 +824,7 @@ const char *find_hook(const char *name)
static struct strbuf path = STRBUF_INIT;
 
strbuf_reset();
-   if (git_hooks_path)
-   strbuf_addf(, "%s/%s", git_hooks_path, name);
-   else
-   strbuf_git_path(, "hooks/%s", name);
+   strbuf_git_path(, "hooks/%s", name);
if (access(path.buf, X_OK) < 0)
return NULL;
return path.buf;
diff --git a/t/t1350-config-hooks-path.sh b/t/t1350-config-hooks-path.sh
index 5e3fb3a..f1f9aee 100755
--- a/t/t1350-config-hooks-path.sh
+++ b/t/t1350-config-hooks-path.sh
@@ -34,4 +34,10 @@ test_expect_success 'Check that various forms of specifying 
core.hooksPath work'
test_cmp expect actual
 '
 
+test_expect_success 'git rev-parse --git-path hooks' '
+   git config core.hooksPath .git/custom-hooks &&
+   git rev-parse --git-path hooks/abc >actual &&
+   test .git/custom-hooks/abc = "$(cat actual)"
+'
+
 test_done
-- 
2.9.2.691.g78954f3

base-commit: 07c92928f2b782330df6e78dd9d019e984d820a7
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] limit the size of the packs we receive

[Jeff King]

On Tue, Aug 16, 2016 at 10:16:58AM +0200, Christian Couder wrote:

> Changes from previous RFC version
> ~
> 
>   - added documentation to all the 3 patches,

Good idea.

>   - changed strtoul() to strtoumax() in the first 2 patches, as
> suggested by Peff,
> 
>   - changed git_config_ulong() to git_config_int64() and used PRIuMAX
> and uintmax_t in the last patch, as suggested by Peff,

Thinking a bit, off_t is actually signed. So maybe PRIdMAX (which we
don't seem to have compat macros for) would make more sense. I dunno if
anybody actually cares. This value shouldn't be signed anyway, and
nobody should be approaching the limits of a 64-bit number anyway (there
is no point in limiting the incoming pack to the exabyte range).

So I'm inclined not to worry about it.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] t/Makefile: make sure that file names are truly platform-independent

[Johannes Schindelin]

Hi Peff & Junio,

On Mon, 15 Aug 2016, Jeff King wrote:

> On Mon, Aug 15, 2016 at 09:57:52AM -0700, Junio C Hamano wrote:
> 
> > I wonder if we already have a good mechanism to allow a project and
> > its participants (say, "me") to declare "in this project, pathnames
> > must conform to this rule" and help them avoid creating a tree that
> > violates the rule customized to their project.
> > 
> > I guess "write_index_as_tree()" would be one of the central places to
> > hook into and that covers an individual contributor or a patch applier
> > who ends up adding offending paths to the project, as well as a merge
> > made in response to a pull request (unless it is a fast-forward)
> > [*1*].  The pre-receive hook can also be used to inspect and reject an
> > attempt to push an offending tree into the history.

FWIW I think it should be at a different level. See below for more
details.

> > Such a mechanism would allow a project that wants participation by
> > folks with case insensitive filesystems to ensure that they do not
> > create a directory that has both xt_TCPMSS.h and xt_tcpmss.h at the
> > same time, for example, but the mechanism needs to allow visibility
> > into more than just a single path when the custom check is made (e.g.
> > a hook run in "write_index_as_tree()" can see all entries in the index
> > to make the decision; if we were to also hook into "add_to_index()",
> > the hook must be able to see other entries in the index to which the
> > new entry is being added).
> 
> I am not convinced this mechanism needs to be built into git. Because it
> happens to be about filenames, git at least has a hope of making sense
> of the various project rules.

Both of you gentle people may recall a conversation in December 2014 when
we scrambled to plug a hole where maliciously-chosen file names would have
allowed to wreak havoc with a local Git repository's config (among other
things).

We did plug it, but not before I proposed to exclude many more file names
than just maliciously-chosen ones. For example, I wanted to exclude all
file names that are illegal on Windows when core.protectNTFS was set to
true.

If we were to implement this "let's help cross-platform projects"
functionality, it would be at that same level.

However, I have to agree with Junio that Git is *not* targeting *all*
platforms. Conversely, any solution we implement to try to be helpful by
pointing out unportable file names will certainly fall short of *some*
project's requirement.

Given that we have no shortage of problems to solve, I would vote for
addressing portability only as far as Git and its intended target
platforms are concerned.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] rev-parse: respect core.hooksPath in --git-path

[Jeff King]

On Tue, Aug 16, 2016 at 02:42:34PM +0200, Johannes Schindelin wrote:

> > I think you can squash in:
> > 
> > diff --git a/run-command.c b/run-command.c
> [...]
> 
> Good point.
> 
> BTW in light of the discussion we are having elsewhere I just need to
> point out that it was *dramatically* faster for me to edit run-command.c,
> find "hooks/" and adjust the code manually than it would have been to save
> the diff and apply it.
> 
> That's because I do not have advanced tooling on top of email (and I also
> could not handle mutt, so I am stuck with a not-really-scriptable email
> client).
> 
> Just sayin'.

There's a flip side, too. It was faster for me to paste in the diff than
it would have been to create a branch, push it to GitHub, and make a PR
on your PR (and then later remember to deal with my PR and branch so as
not to leave them hanging around cluttering up my fork).

I'm definitely sympathetic to people with less exacting email clients,
but I'm not convinced that the GitHub flow is that great if you don't do
the review there in the first place (and even when you do, it's actually
not that great for suggesting things in patch form).

I wonder if public-inbox could have helped here. I think:

  mid=20160815125006.qdssqgd4rzjw4...@sigill.intra.peff.net
  curl http://public-inbox.org/git/$mid/raw | git apply

would work, but the question remains of how you find the right
message-id. You can generally pick it out of the MUA's message headers
manually, though I think some MUAs even make seeing the extended headers
hard. But that's going to be a similar problem with any solution that
isn't your MUA: how do you feed the context of the discussion happening
in one place to another tool.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] rev-parse: respect core.hooksPath in --git-path

[Johannes Schindelin]

Hi Peff,

On Mon, 15 Aug 2016, Jeff King wrote:

> On Mon, Aug 15, 2016 at 02:43:18PM +0200, Johannes Schindelin wrote:
> 
> > The idea of the --git-path option is not only to avoid having to
> > prefix paths with the output of --git-dir all the time, but also to
> > respect overrides for specific common paths inside the .git directory
> > (e.g. `git rev-parse --git-path objects` will report the value of
> > the environment variable GIT_OBJECT_DIRECTORY, if set).
> > 
> > When introducing the core.hooksPath setting, we forgot to adjust
> > git_path() accordingly. This patch fixes that.
> 
> Makes sense.
> 
> I think you can squash in:
> 
> diff --git a/run-command.c b/run-command.c
> index 33bc63a..5a4dbb6 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -824,10 +824,7 @@ const char *find_hook(const char *name)
>   static struct strbuf path = STRBUF_INIT;
>  
>   strbuf_reset();
> - if (git_hooks_path)
> - strbuf_addf(, "%s/%s", git_hooks_path, name);
> - else
> - strbuf_git_path(, "hooks/%s", name);
> + strbuf_git_path(, "hooks/%s", name);
>   if (access(path.buf, X_OK) < 0)
>   return NULL;
>   return path.buf;
> 
> as strbuf_git_path() handles this now.

Good point.

BTW in light of the discussion we are having elsewhere I just need to
point out that it was *dramatically* faster for me to edit run-command.c,
find "hooks/" and adjust the code manually than it would have been to save
the diff and apply it.

That's because I do not have advanced tooling on top of email (and I also
could not handle mutt, so I am stuck with a not-really-scriptable email
client).

Just sayin'.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Minor bug: git config ignores empty sections

[Jeff King]

On Tue, Aug 16, 2016 at 12:06:56AM -0400, Eli Barzilay wrote:

> So it sounds like removing an empty header is problematic in most cases,
> but adding a new variable to an existing empty header should not be...?

I think it's less problematic, though there are still some funny cases.
Like:

  [foo]
  # comments for the "foo" section

  # comments introducing the "bar" section
  [bar]

Ideally the new entry goes in between the two comments. But I don't
think it would be unreasonable to add it right after "[foo]". In this
example, using the blank line as a hint might be useful, but that's
going to be a lot harder to coax out of the callback-parser as syntactic
event.

> I looked at the code, and had a rough sketch that works as follows:
> 
> * Make git_parse_source() call the callback in a special way to note
>   that a section header is seen (I hacked it by passing a special value,
>   a pointer to a global string, as the second argument)

We'd want to make sure that didn't kick in for "regular" callers, with
some kind of option (or, blech, a global variable that changes the
behavior of git_parse_source(), though the config code is already full
of them).

> * Add a new store.last_section_offset field
>
> * In store_aux(), if it's getting the special value, and the section
>   name matches, save the offset in store.last_section_offset

Make sense.

> * In git_config_set_multivar_in_file_gently() right before the "write
>   the first part of the config" comment, test that
>   (store.seen == 1 && copy_begin == 0 && copy_end == contents_sz
>&& store.last_section_offset > 0)
>   and if so, write the contents up to that point, and set copy_begin;
>   if the condition is false, do the same thing it does now

This part I'm not sure about. Naively I'd think you could do with less
special-casing here. If we want to add "foo.two" and find:

  [foo]
  one = whatever

then we mark the end of that line as the point we want to copy up to,
and then add our new "two = ..." line.

Conceptually if we see just:

  [foo]

we should be able to kick in the same code. So it seems like the logic
would all be in the detection and setting of the offset.

But I am not sure I understand all of the code here, and it's rather
complicated. So there is probably a good reason that wouldn't work.

> It looks to me like something like this can work, but it's very hacky
> because I wanted to see if it can work quickly, and because I don't know
> if this kind of a solution will be wanted, and because I don't know
> enough about that code in general.  Making it be an actual solution
> involves a better way to call the callback in a special way (my hack
> does the special thing only if `fn==store_aux`, but it shouldn't),
> calling it after the last variable in a section is seen so it's added
> after that.
> 
> So, will something like this be acceptable?  If so, is there anyone who
> I can ask questions about the code?

Yeah, I think this general strategy is the smallest change that could
work.

What I think would be much more sane in general is to parse the whole
thing into a tree (or even a flat list of events), marking each
syntactically as a section header, a key, a comment, a run of
whitespace, and so on. And then do manipulations on that tree (e.g.,
walk down to the section of interest, add a new key at the end), and
then reformat the tree back into a stream of bytes. That lets you
separate the policy logic from the parsing, and do high-level operations
that might involve multiple passes or backtracking in the tree (e.g.,
walk to the section, walk past any existing keys, walk past any comments
but _not_ past any blank lines, then add the new key).

There are other other upsides, too. For example, the current code cannot
write multiple unrelated keys in a single pass.

The downsides is that it's a complete rewrite of the code. So it's a
_lot_ more work, and it carries more risk of regression. So please don't
take this as "no, your solution isn't OK; you have to do the rewrite".
We've been living with band-aids on the config code for a decade, so I
am OK with one more.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Draft of Git Rev News edition 18

[Christian Couder]

On Mon, Aug 15, 2016 at 8:08 PM, Jakub Narębski  wrote:
> W dniu 14.08.2016 o 23:10, Philip Oakley pisze:
>> From: "Christian Couder" 
>>>
>>> A draft of a new Git Rev News edition is available here:
>>>
>>>
>>> https://github.com/git/git.github.io/blob/master/rev_news/drafts/edition-18.md
>>>
>>> Everyone is welcome to contribute in any section either by editing the
>>> above page on GitHub and sending a pull request, or by commenting on
>>> this GitHub issue:
>>>
>>>  https://github.com/git/git.github.io/issues/170
>>>
>>> You can also reply to this email.
>>
>> I see you mention in passing (well in the small headings near the bottom)
>> that gmane web interface has gone away. It may be worth noting a few of
>> the alternatives, and in particular Eric's newly updated public-inbox
>> https://public-inbox.org/git/.
>
> It would be nice to turn it into mini-story rather than just putting
> a link.

Yeah, it would be also very welcome if someone could send a patch or a
pull request for a mini-story about it.

> Additional references / information:
>
> * "Alternatives to mid.gmane.org?" thread, starting with
>   Message-Id: <481d1ee2-6a66-418f-ab28-95947bbf3...@gmail.com>
>
>   Mentions a few alternatives besides public-inbox:
>
>  https://marc.info/?i=%s
>  https://mid.mail-archive.com/%s
>  https://lists.debian.org/msgid-search/%s
>
>   for finding message based on Message-Id
>
>   NOTE: there are a few mailing list archive sites not mentioned
>   in this thread, like Nabble, some of them listed on (not updated)
>   https://git.wiki.kernel.org/index.php/GitCommunity
>
> * "Ingebrigtsen: The End of Gmane?" short note / article on LWN.net,
>   https://lwn.net/Articles/695695/ [Posted July 28, 2016 by jake],
>   part of "Announcements" (http://lwn.net/Articles/695980/) section
>   of LWN.net Weekly Edition for August 4, 2016
>   (http://lwn.net/Articles/695974/)

Thanks for this. Maybe you could just add those in a pull request or a
patch to the "Git tools and sites" sub section just after the item
about public-inbox?

Thanks anyway!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] t/Makefile: make sure that file names are truly platform-independent

[Junio C Hamano]

Junio C Hamano  writes:

> The trick to catch non-ascii depends on core.quotepath set to true
> (which is the default).  You would need to run ls-files with an
> explicit "-c core.quotepath=false" to be safe.
> ...
> The invocation of "git -c core.quotepath=false ls-files" needs to at
> least have 2>/dev/null to squelch an unnecessary error message.  In
> such a scenario, we may miss a new offending pathname, but we will
> not have false positive and I think that is the best we can do.
>
> Thanks.

Needless to say (1) both "false" should have been "true"; (2) I
shouldn't be crawling out of bed and typing with hazy brain early in
the morning.

Sorry for typoes and thinkos.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] help: make option --help open man pages only for Git commands

[John Keeping]

On Mon, Aug 15, 2016 at 09:40:54PM +0100, Philip Oakley wrote:
> From: "Junio C Hamano" 
> > "Philip Oakley"  writes:
> >
> >> I'm still not sure this is enough. One of the problems back when I
> >> introduced the --guides option (65f9835 (builtin/help.c: add --guide
> >> option, 2013-04-02)) was that we had no easy way of determining what
> >> guides were available, especially given the *nix/Windows split where
> >> the help defaults are different (--man/--html).
> >>
> >> At the time[1] we (I) punted on trying to determine which guides were
> >> actually installed, and just created a short list of the important
> >> guides, which I believe you now check. However the less common guides
> >> are still there (gitcvs-migration?), and others may be added locally.
> >
> > I think we should do both; "git help cvs-migration" should keep the
> > same codeflow and behaviour as we have today (so that it would still
> > work), while "git cvs-migration --help" should say "'cvs-migration'
> > is not a git command".  That would be a good clean-up anyway.
> >
> > It obviously cannot be done if git.c::handle_builtin() does the same
> > "swap  --help to help " hack, but we could improve that
> > part (e.g. rewrite it to "help --swapped " to allow cmd_help()
> > to notice).  When the user said " --help", we don't do guides,
> > when we swapped the word order, we check with guides, too.
> >
> The other option is to simply build a guide-list in exactly the same format 
> as the command list (which if it works can be merged later). Re-use the 
> existing code, etc.

One nice thing at the moment is that third-party Git commands can
install documentation and have "git help" work correctly (shameless plug
for git-integration[1] which does this).  I think Junio's suggestion
above keeps that working whereas having a hardcoded list of guides will
break this.

[1] https://github.com/johnkeeping/git-integration
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] t/Makefile: make sure that file names are truly platform-independent

[Junio C Hamano]

Johannes Schindelin  writes:

>> > > By the way, doesn't ls-files take pathspec glob, saving one extra
>> > > process to run grep?
>> 
>> I specifically did not do that, sorry for omitting the rationale from the
>> commit message. The reason why I have that grep is so that the backslash
>> can also catch non-ASCII characters, such as "Hellö-Jüniö".

That's clever and I did miss that.  It deserves an in-code comment
next to the use of separate grep to prevent others from making the
same mistake in the future.  Having an explanation in the log message
would help but not sufficient for a subtle construct like this.

The trick to catch non-ascii depends on core.quotepath set to true
(which is the default).  You would need to run ls-files with an
explicit "-c core.quotepath=false" to be safe.

It also means that the addition of HT to the grep pattern I
suggested is not necessary, because ls-files will always use "a\tb"
form for a pathname with HT, and you will catch it by looking for
the backslash.  You can lose '"' from the grep pattern for the same
reason.

Two other tricky things are

 (1) the test may be run inside a tarball extract and "git ls-files"
 may not report the pathnames in t/.

 (2) the user may not have a working Git yet in her path.

The invocation of "git -c core.quotepath=false ls-files" needs to at
least have 2>/dev/null to squelch an unnecessary error message.  In
such a scenario, we may miss a new offending pathname, but we will
not have false positive and I think that is the best we can do.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Draft of Git Rev News edition 18

[Josh Triplett]

On Tue, Aug 16, 2016 at 09:30:27AM +, Eric Wong wrote:
> Jakub Narębski  wrote:
> > It's a great pity that https://public-inbox.org/ is just
> > directory index, not a true home page.
> 
> +Cc m...@public-inbox.org
> 
> I'm not sure one could do better while staying true to the
> minimalist nature of plain-text email.
> 
> In the spirit of decentralization, there may not be /a/
> homepage, but many.   Everything is meant to clonable with each
> public-inbox, so maybe every public-inbox will have a code
> branch attached to it with the source+docs bundled.

It'd be nice if it had a prominent list of all lists available; as far
as I can tell, the main page has no link to /git/.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Draft of Git Rev News edition 18

[Eric Wong]

Jakub Narębski  wrote:
> It's a great pity that https://public-inbox.org/ is just
> directory index, not a true home page.

+Cc m...@public-inbox.org

I'm not sure one could do better while staying true to the
minimalist nature of plain-text email.

In the spirit of decentralization, there may not be /a/
homepage, but many.   Everything is meant to clonable with each
public-inbox, so maybe every public-inbox will have a code
branch attached to it with the source+docs bundled.


At least for now, other pages are more easily discoverable[1]
but one day I hope to have repobrowse[2] running there (and
maybe everywhere).


For now, I am prioritizing user/admin/hacker documentation so
more instances can exist.  I'll probably get some online help
stuff up tomorrow.


[1] at least the top-level of public-inbox.org already has more
content than both YHBT.net and bogomips.org combined :)

[2] git clone -b repobrowse https://public-inbox.org/ ...
(idle the moment: think cgit|gitweb with the same (lack of)
 style as public-inbox, +search, +mail thread integration)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [StGit PATCH] contrib/vim: Fix filetype detection in VIM >=7.4

[Catalin Marinas]

On 15 July 2016 at 19:58, Zane Bitter  wrote:
> The command "setfiletype" will not override an existing filetype. This was
> never a problem for me previously, but since upgrading from VIM 7.3 to 7.4
> the filetype for StGit's files is explicitly set to "text", preventing the
> stgit ftdetect plugin overriding it. Use "setlocal filetype=" instead to
> ensure that we override any previously detected filetype.

Applied. Thanks.

Catalin
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] t/Makefile: make sure that paths can be checked out on platforms we care

[Johannes Schindelin]

Some pathnames that are okay on ext4 and on HFS+ cannot be checked
out on Windows. Tests that want to see operations on such paths on
filesystems that support them must do so behind appropriate test
prerequisites, and must not include them in the source tree (instead
they should create them when they run). Otherwise, the source tree
cannot even be checked out.

Make sure that double-quotes, asterisk, colon, greater/less-than,
question-mark, backslash, tab, vertical-bar, as well as any non-ASCII
characters above 0x7f never appear in the pathnames with a new
test-lint-* target as part of a `make test`.

Noticed when a topic wanted to add a pathname with '>' in it.  A
check like this will prevent a similar problems from happening in the
future.

Signed-off-by: Johannes Schindelin 
---

I re-munged the commit message (I saw that you munged it in `pu`
and did not quite like the wording, so I permitted myself the
indulgence).

And of course I tested this, and it does *not* list all files in
subdirectories of t/ as problematic.

Published-As: https://github.com/dscho/git/releases/tag/test-lint-filenames-v2
Fetch-It-Via: git fetch https://github.com/dscho/git test-lint-filenames-v2
Interdiff vs v1:

 diff --git a/t/Makefile b/t/Makefile
 index 3c0eb48..bf9cad9 100644
 --- a/t/Makefile
 +++ b/t/Makefile
 @@ -69,7 +69,7 @@ test-lint-shell-syntax:
@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS)
  
  test-lint-filenames:
 -  @illegal="$$(git ls-files | grep '["*:<>?\\|]')"; \
 +  @illegal="$$(git ls-files | grep '[ "*:<>?\\|]')"; \
test -z "$$illegal" || { \
echo >&2 "illegal file name(s): " $$illegal; exit 1; }
  


 t/Makefile | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/t/Makefile b/t/Makefile
index 18e2b28..bf9cad9 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -52,7 +52,8 @@ clean-except-prove-cache:
 clean: clean-except-prove-cache
$(RM) .prove
 
-test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax
+test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \
+   test-lint-filenames
 
 test-lint-duplicates:
@dups=`echo $(T) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d` && \
@@ -67,6 +68,11 @@ test-lint-executable:
 test-lint-shell-syntax:
@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS)
 
+test-lint-filenames:
+   @illegal="$$(git ls-files | grep '[ "*:<>?\\|]')"; \
+   test -z "$$illegal" || { \
+   echo >&2 "illegal file name(s): " $$illegal; exit 1; }
+
 aggregate-results-and-cleanup: $(T)
$(MAKE) aggregate-results
$(MAKE) clean
-- 
2.9.2.691.g78954f3

base-commit: 70dce4aee55ad8a39a53f86f37a4bd400e0cac7d
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] t/Makefile: make sure that file names are truly platform-independent

[Johannes Schindelin]

Hi Junio,

On Tue, 16 Aug 2016, Johannes Schindelin wrote:

> On Mon, 15 Aug 2016, Junio C Hamano wrote:
> 
> > Junio C Hamano  writes:
> > 
> > >> +test-lint-filenames:
> > >> +@illegal="$$(git ls-files | grep '["*:<>?\\|]')"; \
> > >
> > > This pattern must exclude questionables on either NTFS or HFS+; it
> > > is ironic that it is not even sufficient to limit ourselves to the
> > > Portable Character Set [*1*], but such is life.
> > >
> > > By the way, doesn't ls-files take pathspec glob, saving one extra
> > > process to run grep?
> 
> I specifically did not do that, sorry for omitting the rationale from the
> commit message. The reason why I have that grep is so that the backslash
> can also catch non-ASCII characters, such as "Hellö-Jüniö".

And there is another very good reason to keep the grep: the problem I just
reported is *caused* by your avoiding the grep call.

In my tests, at least, `git ls-files '*\*' lists *all* files in
subdirectories. In other words, it matches the *forward* slash. This also
happens when I run the command in cmd.exe instead of Git Bash, meaning
that it is *not* caused by some MSYS2 path conversion from pseudo-Unix to
Windows paths. That only leaves the conclusion that some of our pathspec
code tries to be helpful and takes a backslash for a directory separator.

In light of these problems, and also in light of the fact that the
test-lint-filenames code is hardly performance critical, I am strongly in
favor of reverting to using grep.

Ciao,
Dscho

Re: [PATCH] t/Makefile: make sure that file names are truly platform-independent

[Johannes Schindelin]

Hi Junio,

On Mon, 15 Aug 2016, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> >> +test-lint-filenames:
> >> +  @illegal="$$(git ls-files | grep '["*:<>?\\|]')"; \
> >
> > This pattern must exclude questionables on either NTFS or HFS+; it
> > is ironic that it is not even sufficient to limit ourselves to the
> > Portable Character Set [*1*], but such is life.
> >
> > By the way, doesn't ls-files take pathspec glob, saving one extra
> > process to run grep?

I specifically did not do that, sorry for omitting the rationale from the
commit message. The reason why I have that grep is so that the backslash
can also catch non-ASCII characters, such as "Hellö-Jüniö".

> One more thing you may want to exclude is HT.  Here is a suggested
> reroll.  I reworded to avoid a subjective "truly platform-independent",
> which is not what we intend to aim for anyway (we only try to support
> the platforms we care about).

Unfortunately, this version (actually, the version of `pu`, which I
assume is identical) does not work correctly:

$ make test-lint
do not use non-portable file name(s): Git-SVN/00compile.t
Git-SVN/Utils/add_path_to_url.t
Git-SVN/Utils/can_compress.t
Git-SVN/Utils/canonicalize_url.t
Git-SVN/Utils/collapse_dotdot.t
Git-SVN/Utils/fatal.t
Git-SVN/Utils/join_paths.t
diff-lib/COPYING
diff-lib/README
helper/.gitignore
helper/test-chmtime.c
helper/test-config.c
helper/test-ctype.c
helper/test-date.c
helper/test-delta.c
helper/test-dump-cache-tree.c
helper/test-dump-split-index.c
helper/test-dump-untracked-cache.c
helper/test-dump-watchman.c
helper/test-fake-ssh.c
helper/test-genrandom.c
helper/test-hashmap.c
helper/test-index-version.c
helper/test-line-buffer.c
helper/test-match-trees.c
helper/test-mergesort.c
helper/test-mktemp.c
helper/test-parse-options.c
helper/test-path-utils.c
helper/test-prio-queue.c
helper/test-read-cache.c
helper/test-regex.c
helper/test-revision-walking.c
helper/test-run-command.c
helper/test-scrap-cache-tree.c
helper/test-sha1-array.c
helper/test-sha1.c
helper/test-sha1.sh
helper/test-sigchain.c
helper/test-string-list.c
helper/test-submodule-config.c
helper/test-subprocess.c
helper/test-svn-fe.c
helper/test-urlmatch-normalization.c
helper/test-wildmatch.c
lib-gpg/keyring.gpg
lib-gpg/ownertrust
lib-httpd/apache.conf
lib-httpd/broken-smart-http.sh
lib-httpd/error.sh
lib-httpd/passwd
lib-httpd/ssl.cnf
perf/.gitignore
perf/Makefile
perf/README
perf/aggregate.perl
perf/min_time.perl
perf/p-perf-lib-sanity.sh
perf/p0001-rev-list.sh
perf/p0002-read-cache.sh
perf/p3400-rebase.sh
perf/p3404-rebase-interactive.sh
perf/p4000-diff-algorithms.sh
perf/p4001-diff-no-index.sh
perf/p4211-line-log.sh
perf/p5302-pack-index.sh
perf/p5303-many-packs.sh
perf/p5310-pack-bitmaps.sh
perf/p7000-filter-branch.sh
perf/p7300-clean.sh
perf/p7810-grep.sh
perf/perf-lib.sh
perf/run
t0110/README
t0110/url-1
t0110/url-10
t0110/url-11
t0110/url-2
t0110/url-3
t0110/url-4
t0110/url-5
t0110/url-6
t0110/url-7
t0110/url-8
t0110/url-9
t0200/test.c
t0200/test.perl
t0200/test.sh
t0202/test.pl
t1509/excludes
t1509/prepare-chroot.sh
t3900/1-UTF-8.txt
t3900/2-UTF-8.txt
t3900/ISO-2022-JP.txt
t3900/ISO8859-1.txt
t3900/UTF-16.txt
t3900/eucJP.txt
t4013/diff.config_format.subjectprefix_DIFFERENT_PREFIX
t4013/diff.diff-tree_--cc_--patch-with-stat_--summary_master
t4013/diff.diff-tree_--cc_--patch-with-stat_--summary_side
t4013/diff.diff-tree_--cc_--patch-with-stat_master
t4013/diff.diff-tree_--cc_--stat_--summary_master
t4013/diff.diff-tree_--cc_--stat_--summary_side
t4013/diff.diff-tree_--cc_--stat_master
t4013/diff.diff-tree_--cc_master
t4013/diff.diff-tree_--patch-with-raw_initial
t4013/diff.diff-tree_--patch-with-stat_initial
t4013/diff.diff-tree_--pretty=oneline_--patch-with-raw_initial
t4013/diff.diff-tree_--pretty=oneline_--patch-with-stat_initial
t4013/diff.diff-tree_--pretty=oneline_--root_--patch-with-raw_initial
t4013/diff.diff-tree_--pretty=oneline_--root_--patch-with-stat_initial
t4013/diff.diff-tree_--pretty=oneline_--root_-p_initial
t4013/diff.diff-tree_--pretty=oneline_--root_initial
t4013/diff.diff-tree_--pretty=oneline_-p_initial
t4013/diff.diff-tree_--pretty=oneline_initial
t4013/diff.diff-tree_--pretty_--patch-with-raw_initial
t4013/diff.diff-tree_--pretty_--patch-with-stat_initial
t4013/diff.diff-tree_--pretty_--patch-with-stat_side
t4013/diff.diff-tree_--pretty_--root_--patch-with-raw_initial
t4013/diff.diff-tree_--pretty_--root_--patch-with-stat_initial
t4013/diff.diff-tree_--pretty_--root_--stat_--summary_initial
t4013/diff.diff-tree_--pretty_--root_--stat_initial
t4013/diff.diff-tree_--pretty_--root_--summary_-r_initial
t4013/diff.diff-tree_--pretty_--root_--summary_initial
t4013/diff.diff-tree_--pretty_--root_-p_initial
t4013/diff.diff-tree_--pretty_--root_initial
t4013/diff.diff-tree_--pretty_--stat_--summary_initial
t4013/diff.diff-tree_--pretty_--stat_initial
t4013/diff.diff-tree_--pretty_--summary_initial
t4013/diff.diff-tree_--pretty_-p_initial

Re: [RFC/PATCH 3/3] receive-pack: allow a maximum input size to be specified

[Christian Couder]

On Tue, Aug 16, 2016 at 3:03 AM, Jeff King  wrote:
> On Mon, Aug 15, 2016 at 03:48:55PM -0700, Junio C Hamano wrote:
>
>> Jeff King  writes:
>>
>> > The simple fix is to call register_tempfile() in open_pack_file(), and
>> > just have index-pack clean up the file on its way out.
>> >
>> > But there are harder cases. For instance, imagine somebody pushes a
>> > 500MB file, and you have a pre-receive hook that says "too big; I won't
>> > accept this". And then they push in a loop, as before. You've accepted
>> > the incoming pack into the repository by the time the pre-receive runs.
>> > You can't just delete it, because you don't know if other simultaneous
>> > processes have started to depend on the objects.
>> >
>> > To solve that, I have patches that put incoming packfiles into a
>> > "quarantine" area, then run the connectivity check and pre-receive hooks
>> > with the quarantine accessible via GIT_ALTERNATE_OBJECT_DIRECTORIES. And
>> > then we either move the quarantine packs into the real repo, or blow
>> > away the tmpdir, depending on whether the hooks said the objects were
>> > OK.

Thanks Peff for the review and all these explanations and suggestions!

>> > Those are patches I plan to share upstream but just haven't gotten
>> > around to yet.

I would be happy to help if I can on these patches too!

>> I think these other patches can come later, independent from this
>> three-patch series resurrected from the archive, so I can take a
>> reroll of these once the integer-size issues you pointed out are
>> sorted out.

Ok, I just sent a non RFC reroll of the series with those issues
hopefully fixed.

> Yeah, definitely it's independent. I was mostly suggesting to Christian
> that he may want to look into the "easy" register_tempfile() case, as
> GitLab may find this is only half of the fix that they want.

Yeah, thanks again for this suggestion!

> Also a patch I may try to polish and share in the future, but not as
> high priority as some of the other stuff.

I can help polishing this patch if you want.

Thanks,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git cherry-pick conflict error message is deceptive when cherry-picking multiple commits

[Remi Galan Alfonso]

Hi Stephen,

Stephen Morton  writes:
> +if  (multiple_commits)
> +   advise(_("after resolving the conflicts,
> mark the corrected paths with 'git add ' or 'git rm '\n"
> +"then continue with 'git %s
> --continue'\n"
> +"or cancel with 'git %s
> --abort'" ), action_name(opts), action_name(opts));
> +else
> +advise(_("after resolving the
> conflicts, mark the corrected paths\n"
> +"with 'git add ' or 'git
> rm '\n"
> +"and commit the result with
> 'git commit'"));

In both cases (multiple_commits or not), the beginning of the advise
is nearly the same, with only a '\n' in the middle being the
difference:

multiple_commits:
 "after resolving the conflicts, mark the corrected paths with 'git
 add ' or 'git rm '\n"

!multiple_commits:
 "after resolving the conflicts, mark the corrected paths\n with 'git
 add ' or 'git rm '\n"
  ~~~^

In 'multiple_commits' case the advise is more than 80 characters long,
did you forget the '\n' in that case?

If you end up using the same beginning of advice, maybe it's possible
to give it before the 'if(multiple_commits)' and avoid duplication of
the lines.

Thanks,
Rémi
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] unpack-objects: add --max-input-size= option

[Christian Couder]

When receiving a pack-file, it can be useful to abort the
`git unpack-objects`, if the pack-file is too big.

Signed-off-by: Christian Couder 
---
 Documentation/git-unpack-objects.txt | 3 +++
 builtin/unpack-objects.c | 7 +++
 2 files changed, 10 insertions(+)

diff --git a/Documentation/git-unpack-objects.txt 
b/Documentation/git-unpack-objects.txt
index 3e887d1..b3de50d 100644
--- a/Documentation/git-unpack-objects.txt
+++ b/Documentation/git-unpack-objects.txt
@@ -44,6 +44,9 @@ OPTIONS
 --strict::
Don't write objects with broken content or links.
 
+--max-input-size=::
+   Die, if the pack is larger than .
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 172470b..4532aa0 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -19,6 +19,7 @@ static const char unpack_usage[] = "git unpack-objects [-n] 
[-q] [-r] [--strict]
 static unsigned char buffer[4096];
 static unsigned int offset, len;
 static off_t consumed_bytes;
+static off_t max_input_size;
 static git_SHA_CTX ctx;
 static struct fsck_options fsck_options = FSCK_OPTIONS_STRICT;
 
@@ -87,6 +88,8 @@ static void use(int bytes)
if (signed_add_overflows(consumed_bytes, bytes))
die("pack too large for current definition of off_t");
consumed_bytes += bytes;
+   if (max_input_size && consumed_bytes > max_input_size)
+   die(_("pack exceeds maximum allowed size"));
 }
 
 static void *get_data(unsigned long size)
@@ -550,6 +553,10 @@ int cmd_unpack_objects(int argc, const char **argv, const 
char *prefix)
len = sizeof(*hdr);
continue;
}
+   if (skip_prefix(arg, "--max-input-size=", )) {
+   max_input_size = strtoumax(arg, NULL, 10);
+   continue;
+   }
usage(unpack_usage);
}
 
-- 
2.10.0.rc0.3.g8535b4c

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] receive-pack: allow a maximum input size to be specified

[Christian Couder]

From: Jeff King 

Receive-pack feeds its input to either index-pack or
unpack-objects, which will happily accept as many bytes as
a sender is willing to provide. Let's allow an arbitrary
cutoff point where we will stop writing bytes to disk.

Cleaning up what has already been written to disk is a
related problem that is not addressed by this patch.

The problem is that git-gc can clean up tmp_pack_* files
after their grace time expired, but that may not be
enough if someone tries to "git push" in a loop.

A simple fix is to call register_tempfile() in
open_pack_file(), and just have index-pack clean up the
file on its way out.

But there are harder cases. For instance, if somebody
pushes a 500MB file, and there is a pre-receive hook that
says "too big; I won't accept this". And then they push in
a loop, the incoming pack has already been accepted into
the repository by the time the pre-receive hook runs.
It's not possible to just delete it, because we don't know
if other simultaneous processes have started to depend on
the objects.

Signed-off-by: Jeff King 
Signed-off-by: Christian Couder 
---
 Documentation/config.txt   |  5 +
 Documentation/git-receive-pack.txt |  3 +++
 builtin/receive-pack.c | 12 +++
 t/t5546-push-limits.sh | 42 ++
 4 files changed, 62 insertions(+)
 create mode 100755 t/t5546-push-limits.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0bcb679..f5b6061 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2517,6 +2517,11 @@ receive.unpackLimit::
especially on slow filesystems.  If not set, the value of
`transfer.unpackLimit` is used instead.
 
+receive.maxsize::
+   If the size of a pack file is larger than this limit, then
+   git-receive-pack will error out, instead of accepting the pack
+   file. If not set or set to 0, then the size is unlimited.
+
 receive.denyDeletes::
If set to true, git-receive-pack will deny a ref update that deletes
the ref. Use this to prevent such a ref deletion via a push.
diff --git a/Documentation/git-receive-pack.txt 
b/Documentation/git-receive-pack.txt
index 000ee8d..0ccd5fb 100644
--- a/Documentation/git-receive-pack.txt
+++ b/Documentation/git-receive-pack.txt
@@ -33,6 +33,9 @@ post-update hooks found in the Documentation/howto directory.
 option, which tells it if updates to a ref should be denied if they
 are not fast-forwards.
 
+A number of other receive.* config options are available to tweak
+its behavior, see linkgit:git-config[1].
+
 OPTIONS
 ---
 ::
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 92e1213..8c2943d 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -46,6 +46,7 @@ static int transfer_unpack_limit = -1;
 static int advertise_atomic_push = 1;
 static int advertise_push_options;
 static int unpack_limit = 100;
+static off_t max_input_size;
 static int report_status;
 static int use_sideband;
 static int use_atomic;
@@ -212,6 +213,11 @@ static int receive_pack_config(const char *var, const char 
*value, void *cb)
return 0;
}
 
+   if (strcmp(var, "receive.maxsize") == 0) {
+   max_input_size = git_config_int64(var, value);
+   return 0;
+   }
+
return git_default_config(var, value, cb);
 }
 
@@ -1650,6 +1656,9 @@ static const char *unpack(int err_fd, struct shallow_info 
*si)
if (fsck_objects)
argv_array_pushf(, "--strict%s",
fsck_msg_types.buf);
+   if (max_input_size)
+   argv_array_pushf(, 
"--max-input-size=%"PRIuMAX,
+   (uintmax_t)max_input_size);
child.no_stdout = 1;
child.err = err_fd;
child.git_cmd = 1;
@@ -1678,6 +1687,9 @@ static const char *unpack(int err_fd, struct shallow_info 
*si)
fsck_msg_types.buf);
if (!reject_thin)
argv_array_push(, "--fix-thin");
+   if (max_input_size)
+   argv_array_pushf(, 
"--max-input-size=%"PRIuMAX,
+   (uintmax_t)max_input_size);
child.out = -1;
child.err = err_fd;
child.git_cmd = 1;
diff --git a/t/t5546-push-limits.sh b/t/t5546-push-limits.sh
new file mode 100755
index 000..b38d508
--- /dev/null
+++ b/t/t5546-push-limits.sh
@@ -0,0 +1,42 @@
+#!/bin/sh
+
+test_description='check input limits for pushing'
+. ./test-lib.sh
+
+test_expect_success 'create remote repository' '
+   git init --bare dest
+'
+
+# Let's run tests with different unpack limits: 1 and 10
+# When the limit is 1, `git receive-pack` will call `git index-pack`.
+# When the limit is 10, `git receive-pack` will call `git 

[PATCH 0/3] limit the size of the packs we receive

[Christian Couder]

Goal


In https://public-inbox.org/git/20150612182045.GA23698%40peff.net/,
Peff sent a patch that is used by GitHub to abort `git receive-pack`
when the size of the pack we receive is bigger than a configured
limit.

GitLab is interested in using the same approach and in standardizing
the error messages the user could get back.

Comments


I kept Peff as the author of the patches that are made mostly from his
patch, but I added my Signed-off-by to them.

Changes from previous RFC version
~

  - added documentation to all the 3 patches,

  - changed strtoul() to strtoumax() in the first 2 patches, as
suggested by Peff,

  - changed git_config_ulong() to git_config_int64() and used PRIuMAX
and uintmax_t in the last patch, as suggested by Peff,

  - reorganized the tests in the last patch, as suggested by Peff

  - improved commit message of the last patch with information from
Peff

Links
~

This patch series is available here:

https://github.com/chriscool/git/commits/max-receive

The previous RFC version is here on GitHub:

https://github.com/chriscool/git/commits/max-receive2

and here on the list:

https://public-inbox.org/git/20160815195729.16826-1-chrisc...@tuxfamily.org/

Peff's initial patch is:

https://public-inbox.org/git/20150612182045.GA23698%40peff.net/

Diff with previous RFC version
~~

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0bcb679..f5b6061 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2517,6 +2517,11 @@ receive.unpackLimit::
especially on slow filesystems.  If not set, the value of
`transfer.unpackLimit` is used instead.
 
+receive.maxsize::
+   If the size of a pack file is larger than this limit, then
+   git-receive-pack will error out, instead of accepting the pack
+   file. If not set or set to 0, then the size is unlimited.
+
 receive.denyDeletes::
If set to true, git-receive-pack will deny a ref update that deletes
the ref. Use this to prevent such a ref deletion via a push.
diff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt
index 7a4e055..1b4b65d 100644
--- a/Documentation/git-index-pack.txt
+++ b/Documentation/git-index-pack.txt
@@ -87,6 +87,8 @@ OPTIONS
Specifying 0 will cause Git to auto-detect the number of CPU's
and use maximum 3 threads.
 
+--max-input-size=::
+   Die, if the pack is larger than .
 
 Note
 
diff --git a/Documentation/git-receive-pack.txt 
b/Documentation/git-receive-pack.txt
index 000ee8d..0ccd5fb 100644
--- a/Documentation/git-receive-pack.txt
+++ b/Documentation/git-receive-pack.txt
@@ -33,6 +33,9 @@ post-update hooks found in the Documentation/howto directory.
 option, which tells it if updates to a ref should be denied if they
 are not fast-forwards.
 
+A number of other receive.* config options are available to tweak
+its behavior, see linkgit:git-config[1].
+
 OPTIONS
 ---
 ::
diff --git a/Documentation/git-unpack-objects.txt 
b/Documentation/git-unpack-objects.txt
index 3e887d1..b3de50d 100644
--- a/Documentation/git-unpack-objects.txt
+++ b/Documentation/git-unpack-objects.txt
@@ -44,6 +44,9 @@ OPTIONS
 --strict::
Don't write objects with broken content or links.
 
+--max-input-size=::
+   Die, if the pack is larger than .
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 1fd60bd..4a8b4ae 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1718,7 +1718,7 @@ int cmd_index_pack(int argc, const char **argv, const 
char *prefix)
if (*c || opts.off32_limit & 0x8000)
die(_("bad %s"), arg);
} else if (skip_prefix(arg, "--max-input-size=", )) 
{
-   max_input_size = strtoul(arg, NULL, 10);
+   max_input_size = strtoumax(arg, NULL, 10);
} else
usage(index_pack_usage);
continue;
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 7627f7f..8c2943d 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -214,7 +214,7 @@ static int receive_pack_config(const char *var, const char 
*value, void *cb)
}
 
if (strcmp(var, "receive.maxsize") == 0) {
-   max_input_size = git_config_ulong(var, value);
+   max_input_size = git_config_int64(var, value);
return 0;
}
 
@@ -1657,8 +1657,8 @@ static const char *unpack(int err_fd, struct shallow_info 
*si)
argv_array_pushf(, "--strict%s",
fsck_msg_types.buf);
if (max_input_size)
-   argv_array_pushf(, "--max-input-size=%lu",
-   max_input_size);
+  

[PATCH 1/3] index-pack: add --max-input-size= option

[Christian Couder]

From: Jeff King 

When receiving a pack-file, it can be useful to abort the
`git index-pack`, if the pack-file is too big.

Signed-off-by: Jeff King 
Signed-off-by: Christian Couder 
---
 Documentation/git-index-pack.txt | 2 ++
 builtin/index-pack.c | 5 +
 2 files changed, 7 insertions(+)

diff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt
index 7a4e055..1b4b65d 100644
--- a/Documentation/git-index-pack.txt
+++ b/Documentation/git-index-pack.txt
@@ -87,6 +87,8 @@ OPTIONS
Specifying 0 will cause Git to auto-detect the number of CPU's
and use maximum 3 threads.
 
+--max-input-size=::
+   Die, if the pack is larger than .
 
 Note
 
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 1d2ea58..4a8b4ae 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -87,6 +87,7 @@ static struct progress *progress;
 static unsigned char input_buffer[4096];
 static unsigned int input_offset, input_len;
 static off_t consumed_bytes;
+static off_t max_input_size;
 static unsigned deepest_delta;
 static git_SHA_CTX input_ctx;
 static uint32_t input_crc32;
@@ -297,6 +298,8 @@ static void use(int bytes)
if (signed_add_overflows(consumed_bytes, bytes))
die(_("pack too large for current definition of off_t"));
consumed_bytes += bytes;
+   if (max_input_size && consumed_bytes > max_input_size)
+   die(_("pack exceeds maximum allowed size"));
 }
 
 static const char *open_pack_file(const char *pack_name)
@@ -1714,6 +1717,8 @@ int cmd_index_pack(int argc, const char **argv, const 
char *prefix)
opts.off32_limit = strtoul(c+1, , 0);
if (*c || opts.off32_limit & 0x8000)
die(_("bad %s"), arg);
+   } else if (skip_prefix(arg, "--max-input-size=", )) 
{
+   max_input_size = strtoumax(arg, NULL, 10);
} else
usage(index_pack_usage);
continue;
-- 
2.10.0.rc0.3.g8535b4c

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: storing cover letter of a patch series?

[Duy Nguyen]

On Tue, Aug 16, 2016 at 12:26 PM, Jacob Keller  wrote:
>>> They can just add "squash! cover! " commits for that ;-) Though more
>>> likely the advanced workflow would be used... We'll need both (more than
>>> one) options.
>>
>> Or even better, "git commit --reword $SHA1" brings up the editor with
>> commit message of $SHA1. Modify any way you want and it creates a new
>> empty, "reword!" commit that contains the diff between the old commit
>> message and the new one. "reword!" can be consumed by "rebase -i
>> --autosquash" without bringing up the editor again. I realize making
>> "git commit --reword" run multiple times would be tricky though...
>
> I was just thinking you write text and it gets appended to the text of
> the reworded commit, and when you squash them using rebase you get to
> finalize it like a normal squash?

I think that's what Phillip meant by 'squash! cover!' though I wanted
to go further, I don't want an editor popping up at rebase time,
instead 'rebase' just update cover letter automatically for me.
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html