Re: [PATCH 0/6] clean up parsing of maybe_bool

[Martin Ågren]

On 7 August 2017 at 23:12, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> The series looks fine to me overall, though patch 5 is overly gentle IMHO.
>> We could have removed it right there as Junio is very good at resolving
>> conflicts or producing dirty merges for such a situation.
>> But delaying it until no other series' are in flight is fine with me, too.
>
[...]
>
> I am fine with either in this case, but I probably would have opted
> for removal at the end of this series if I were doing this series,
> because
>
> -   git_config_maybe_bool(K,V)
> +   git_parse_maybe_bool(V)
>
> that may have to happen during evil merges would have been trivial.

Thanks, both of you. I could wait a couple of days to see if there are
other things to address, then send a v2 with a more aggressive patch 5?

Martin

Re: [PATCH] git svn fetch: Create correct commit timestamp when using --localtime

[Eric Wong]

Junio C Hamano  wrote:
> Urs Thuermann  writes:
> 
> > In parse_svn_date() prepend the correct UTC offset to the timestamp
> > returned.  This is the offset in effect at the commit time instead of
> > the offset in effect at calling time.
> >
> > Signed-off-by: Urs Thuermann 
> > ---
> >  perl/Git/SVN.pm | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Thanks; sounds sensible.  
> 
> Eric?

Yep, seems alright.  Can you apply directly?
Been a bit preoccupied as of late.  Thanks.

Re:

[Mrs . Marie Angèle O]

-- 
Permit me to communicate with you in private.

I have a profitable transaction to discuss with you and i will
disclose the details once i receive your acceptance reply.

Re: [PATCH] Fix delta integer overflows

[Junio C Hamano]

Junio C Hamano  writes:

> Martin Koegler  writes:
>
>> From: Martin Koegler 
>>
>> The current delta code produces incorrect pack objects for files > 4GB.
>>
>> Signed-off-by: Martin Koegler 
>> ---
>>  diff-delta.c | 23 ---
>>  1 file changed, 12 insertions(+), 11 deletions(-)
>>
>> Just pass any file > 4 GB to the delta-compression [by increasing the delta 
>> limits].
>> As file size, a truncated 32bit value will be encoded, leading to broken 
>> pack files.
>
> The patch obviously makes the code better and self consistent in
> that "struct delta_index" has src_size as ulong, and this function
> takes trg_size as ulong, and it was plain wrong for the code to
> assume that "i", which is uint, can receive it safely.
>
> In the longer term we might want to move to size_t or even
> uintmax_t, as the ulong on a platform may not be long enough in
> order to express the largest file size the platform can have, but
> this patch (1) is good even without such a change, and (2) gives a
> good foundation to build on if we want such a change on top.
>
> Thanks.  Will queue.

Having said that, I am a bit curious how you came to this patch.
Was the issue found by code inspection, or did you actually have a
real life use case to raise the core.bigFileThreshold configuration
to a value above 4GB?

Thanks.

[RFC] clang-format: outline the git project's coding style

[Brandon Williams]

Add a '.clang-format' file which outlines the git project's coding
style.  This can be used with clang-format to auto-format .c and .h
files to conform with git's style.

Signed-off-by: Brandon Williams 
---

I'm sure this sort of thing comes up every so often on the list but back at
git-merge I mentioned how it would be nice to not have to worry about style
when reviewing patches as that is something mechanical and best left to a
machine (for the most part).  I saw that 'clang-format' was brought up on the
list once before a couple years ago
(https://public-inbox.org/git/20150121220903.ga10...@peff.net/) but nothing
really came of it.  I spent a little bit of time combing through the various
options and came up with this config based on the general style of our code
base.  The big issue though is that our code base isn't consistent so try as
you might you wont be able to come up with a config which matches everything we
do (mostly due to the inconsistencies in our code base).

Anyway, I thought I'd bring this topic back up and see how people feel about it.

 .clang-format | 166 ++
 1 file changed, 166 insertions(+)
 create mode 100644 .clang-format

diff --git a/.clang-format b/.clang-format
new file mode 100644
index 0..7f28dc259
--- /dev/null
+++ b/.clang-format
@@ -0,0 +1,166 @@
+# Defaults
+
+# Use tabs whenever we need to fill whitespace that spans at least from one tab
+# stop to the next one.
+UseTab: Always
+TabWidth: 8
+IndentWidth: 8
+ContinuationIndentWidth: 8
+ColumnLimit: 80
+
+# C Language specifics
+Language: Cpp
+
+# Align parameters on the open bracket
+# someLongFunction(argument1,
+#  argument2);
+AlignAfterOpenBracket: Align
+
+# Don't align consecutive assignments
+# int  = 12;
+# int b = 14;
+AlignConsecutiveAssignments: false
+
+# Don't align consecutive declarations
+# int  = 12;
+# double b = 3.14;
+AlignConsecutiveDeclarations: false
+
+# Align escaped newlines as far left as possible
+# #define A   \
+#   int ; \
+#   int b;\
+#   int ;
+AlignEscapedNewlines: Left
+
+# Align operands of binary and ternary expressions
+# int aaa = bbb +
+#   cc;
+AlignOperands: true
+
+# Don't align trailing comments
+# int a; // Comment a
+# int b = 2; // Comment b
+AlignTrailingComments: false
+
+# By default don't allow putting parameters onto the next line
+# myFunction(foo, bar, baz);
+AllowAllParametersOfDeclarationOnNextLine: false
+
+# Don't allow short braced statements to be on a single line
+# if (a)   not   if (a) return;
+#   return;
+AllowShortBlocksOnASingleLine: false
+AllowShortCaseLabelsOnASingleLine: false
+AllowShortFunctionsOnASingleLine: false
+AllowShortIfStatementsOnASingleLine: false
+AllowShortLoopsOnASingleLine: false
+
+# Add a line break after the return type of top-level functions
+# int
+# foo();
+AlwaysBreakAfterReturnType: TopLevel
+
+# Pack as many parameters or arguments onto the same line as possible
+# int myFunction(int , int ,
+#int );
+BinPackArguments: true
+BinPackParameters: true
+
+# Attach braces to surrounding context except break before braces on function
+# definitions.
+# void foo()
+# {
+#if (true) {
+#} else {
+#}
+# };
+BreakBeforeBraces: Linux
+
+# Break after operators
+# int valuve = a +
+#  bb -
+#  ccc;
+BreakBeforeBinaryOperators: None
+BreakBeforeTernaryOperators: false
+
+# Don't break string literals
+BreakStringLiterals: false
+
+# Use the same indentation level as for the switch statement.
+# Switch statement body is always indented one level more than case labels.
+IndentCaseLabels: false
+
+# Don't indent a function definition or declaration if it is wrapped after the
+# type
+IndentWrappedFunctionNames: false
+
+# Align pointer to the right
+# int *a;
+PointerAlignment: Right
+
+# Insert a space after a cast
+# x = (int32) y;notx = (int32)y;
+SpaceAfterCStyleCast: true
+
+# Insert spaces before and after assignment operators
+# int a = 5;notint a=5;
+# a += 42; a+=42;
+SpaceBeforeAssignmentOperators: true
+
+# Put a space before opening parentheses only after control statement keywords.
+# void f() {
+#   if (true) {
+# f();
+#   }
+# }
+SpaceBeforeParens: ControlStatements
+
+# Don't insert spaces inside empty '()'
+SpaceInEmptyParentheses: false
+
+# The number of spaces before trailing line comments (// - comments).
+# This does not affect trailing block comments (/* - comments).
+SpacesBeforeTrailingComments: 1
+
+# Don't insert spaces in casts
+# x = (int32) y;notx = ( int32 ) y;
+SpacesInCStyleCastParentheses: false
+
+# Don't insert spaces inside container literals
+# var arr = [1, 2, 3];notvar arr = [ 1, 2, 3 ];
+SpacesInContainerLiterals: false
+
+# Don't insert spaces after '(' or before ')'
+# f(arg);notf( arg );

RE: upgarding GIT

[James Wells]

Hi Todd

Thanks for replying, below is my current install information

Current ( STASH and GIT are installed on the same server ):

STASH ( BitBucket ) = 3.9.2
Git = 2.0.4 ( installed from tar Ball and not from an RPM as the RPM was too 
old.
Centos = 6.6

Required:

BitBucket = 5.2
Git = 2.2 + and above

https://confluence.atlassian.com/bitbucketserver/supported-platforms-776640981.html

As you can see my version of git is not supported with the current version of 
bitbucket. I will have to perform a two stage upgrade anyway as the version of 
STASH I am running cannot be upgraded directly to bitbucket 5.2 as well.

Is there an easy way just to install a higher support version of git like 2.9 
on the same server and then move all the repos and basically everything across. 
Can you install another TAR ball later version on top of another git , so it's 
like overwriting it. 


Kind regards,

James Wells | Operations and Regional Account Manager ANZ    

www.statseeker.com

This email is intended only for the entity or individual to whom it is 
addressed and may contain information that is privileged or confidential. If 
you are not the intended recipient, you are hereby notified that distribution, 
copying or any form of dissemination of the content of this email is strictly 
prohibited. If you have received this email in error, please advise us 
immediately by return email and destroy the original message. Thank you.


-Original Message-
From: Todd Zullinger [mailto:todd.zullin...@gmail.com] On Behalf Of Todd 
Zullinger
Sent: Tuesday, 8 August 2017 3:08 AM
To: Ævar Arnfjörð Bjarmason
Cc: James Wells; git@vger.kernel.org
Subject: Re: upgarding GIT

Ævar Arnfjörð Bjarmason wrote:
> On Mon, Aug 07 2017, James Wells jotted:
>> I am fairly new to git, however I have a challenge of upgrading git 
>> from 2.0.4 to 2.4.12 and my initial 2.0.4 install was done via TAR 
>> BALL on my server.
>>
>> I have a centos server running git and Atlassian STASH and my 
>> challenge is for me to upgrade the STASH application I need to move 
>> to a newer version of git.

Which release of CentOS are you using James?  And what git version is required 
for the Atlassian Stash (which is now called Bitbucket) release you're trying 
to use?  IIRC, they support as far back as git 1.8?

> You're going to want to install git via RPM/yum. CentOS already has a 
> package for it.

Indeed.  (But I'm biased because I would never, ever install software via 'sudo 
make install' on anything other than a throw-away test
instance.)

The one problem folks run into on CentOS/RHEL is that the versions may be 
somewhat old.  CentOS/RHEL 6 ships with git 1.7.1, for instance. 
CentOS/RHEL 7 is only a little newer, with git 1.8.3.  There are "software 
collections" which provide git 1.9¹ and 2.9², but personally I've never liked 
using software collections for software that I need to integrate with other 
tools.

For users of CentOS/RHEL who want to run the current git release in a packaged 
form, the Fedora git package maintainers take care to ensure that the Fedora 
packages can be built for the current supported releases of CentOS/RHEL (6 & 7 
at the moment).  Grabbing the current code and/or srpm from Fedora should 
(almost³) always build cleanly using the mock build tool for CentOS/RHEL.

I also try to keep a semi-official COPR repo up to date, here:

https://copr.fedorainfracloud.org/coprs/g/git-maint/git/

(Even as the primary maintainer of that repo, I'd still suggest that it's wise 
to either mirror it locally or rebuild the srpm's in your own infrastructure, 
to ensure that if the copr service is ever down you can reinstall important 
systems.)

¹ https://www.softwarecollections.org/en/scls/rhscl/git19/
² https://www.softwarecollections.org/en/scls/rhscl/rh-git29/
³ Right now, there's a slight issue building the current git for
  CentOS 7 because RHEL 7.4 moved the pcre2 package from EPEL into
  RHEL and CentOS 7.4 is not yet built.  The Fedora packages are
  built against pcre2 now (thanks Ævar ;).  So for a few weeks it
  won't be possible to build them for CentOS 7 without a minor change.

-- 
Todd
~~
Ocean, n. A body of water occupying about two-thirds of a world made
for man -- who has no gills.
-- Ambrose Bierce, "The Devil's Dictionary"

Re: Partial clone design (with connectivity check for locally-created objects)

[Jonathan Tan]

On Mon, 7 Aug 2017 15:12:11 -0400
Ben Peart  wrote:

> I missed the offline discussion and so am trying to piece together what 
> this latest design is trying to do.  Please let me know if I'm not 
> understanding something correctly.
> 
>  From what I can tell, objects are going to be segmented into two 
> "types" - those that were fetched from a remote source that allows 
> partial clones/fetches (lazyobject/imported) and those that come from 
> "regular" remote sources (homegrown) that requires all objects to exist 
> locally.
> 
> FWIW, the names here are not making things clearer for me. If I'm 
> correct perhaps "partial" and "normal" would be better to indicate the 
> type of the source? Anyway...

That's right. As for names, I'm leaning now towards "imported" and
"non-imported". "Partial" is a bit strange because such an object is
fully available; it's just that the objects that it references are
promised by the server.

> Once the objects are segmented into the 2 types, the fsck connectivity 
> check code is updated to ignore missing objects from "partial" remotes 
> but still expect/validate them from "normal" remotes.
> 
> This compromise seems reasonable - don't generate errors for missing 
> objects for remotes that returned a partial clone but do generate errors 
> for missing objects from normal clones as a missing object is always an 
> error in this case.

Yes. In addition, the references of "imported" objects are also
potentially used when connectivity-checking "non-imported" objects - if
a "non-imported" object refers to an object that an "imported" object
refers to, that is fine, even though we don't have that object.

> This segmentation is what is driving the need for the object loader to 
> build a new local pack file for every command that has to fetch a 
> missing object.  For example, we can't just write a tree object from a 
> "partial" clone into the loose object store as we have no way for fsck 
> to treat them differently and ignore any missing objects referenced by 
> that tree object.
> 
> My concern with this proposal is the combination of 1) writing a new 
> pack file for every git command that ends up bringing down a missing 
> object and 2) gc not compressing those pack files into a single pack file.
> 
> We all know that git doesn't scale well with a lot of pack files as it 
> has to do a linear search through all the pack files when attempting to 
> find an object.  I can see that very quickly, there would be a lot of 
> pack files generated and with gc ignoring "partial" pack files, this 
> would never get corrected.
> 
> In our usage scenarios, _all_ of the objects come from "partial" clones 
> so all of our objects would end up in a series of "partial" pack files 
> and would have pretty poor performance as a result.

One possible solution...would support for annotating loose objects with
".remote" be sufficient? (That is, for each loose object file created,
create another of the same name but with ".remote" appended.) This means
that a loose-object-creating lazy loader would need to create 2 files
per object instead of one.

The lazy loader protocol will thus be updated to something resembling a
prior version with the loader writing objects directly to the object
database, but now the loader is also responsible for creating the
".remote" files.  (In the Android use case, we probably won't need the
writing-to-partial-packfile mechanism anymore since only comparatively
few and large blobs will go in there.)

Re: [PATCH] Fix delta integer overflows

[Junio C Hamano]

Junio C Hamano  writes:

> This is not about where the bar is set.  It is about expectation

After having thought about this a bit more, I think in the message I
am responding to I mischaracterised the aspect of a patch that
influences the "expectation".  It is much less about who the
contributor is but more about what the patch does.  

If the patch in question were from a more experienced contributor
(like you or Peff), my internal reaction would have been "gee, the
submitter should have known better that a more complete fix should
involve a larger integral type, not stopping at matching the largest
type that happens to be used in the interface without updating the
interface".

But I still would have said that the patch is an improvement--as it
indeed is; it does not make things worse anywhere and brings in a
more consistency.  And I still would have mentioned the same "in the
longer term, we would want to use size_t or uintmax_t here, not just
ulong".

The only thing I would have done differently if the submission were
by a more experienced contributor is that I probably would have
added "yes this may be an improvement, but I expected you should
know better to at least mention the longer term direction to use
size_t or uintmax_t in the log message, even if you didn't
immediately extend this patch into a more complete series".

That one is a difference of expectation between an occasional
contributor and an experienced one.

Re: [suggestion] Include commit-ish in git status output

[Junio C Hamano]

Mahmoud Al-Qudsi  writes:

> Looking back, I probably should have started with that. `git
> status` gives the status of the _relative_ current state of the
> local repository without printing any information that can be used
> as an _absolute_ reference to "frame" the results of the `git
> status` command.

Yeah, that may be a good characterization of what 'git status'
does.  Another thing is that it gives only a summary.  It may say
"this and that path were changed", but it does not exactly say "how"
they were changed.  So from that point of view, even if you know
which absolute reference the status of the working tree and the
index being reported is relative to, it still does not give you
much for you to be able to reproduce the exact state.  That is not
the purpose of the tool---it is to help the user who is aware of
where s/he started from.

> If I run `git status`, make, commit, and push some changes, then
> run `git status` once more, the output of the command can be
> identical to the previous run, _even though the actual state of
> the repo has changed_ which is... less than useful and potentially
> misleading.

I do not quite understand why it is misleading.

Re: Suggestion: better error message when an ambiguous checkout is executed

[Junio C Hamano]

Mahmoud Al-Qudsi  writes:

> The default git behavior when attempting to `git checkout xxx` for
> some value of "xxx" that cannot be resolved to a single, unique
> file/path/branch/tag/commit/etc is to display the following:
>
>> error: pathspec 'xxx' did not match any file(s) known to git

Yes, it is true that the user may have wanted to instead checkout a
branch 'xxy' and misspelled it as 'xxx'.  Or the user may have more
than one remotes, from which there are remote-tracking branches for
'xxx' branch.  Neither of these cases allow Git to interpret 'xxx'
as a rev, and Git blindly thinks that 'xxx' must be a pathspec, and
wants to ensure that such a path exists in the working tree (if
'xxx' does not look like a wildcard or otherwise magic pathspec).

> Unfortunately, this is (IMHO) at best misleading when the actual case
> is that "git could not unambiguously resolve pathspec xxx"

The actual case you want to address is "git could not tell that the
user meant 'xxx' as a revision, even though the end user meant it as
such".

> Can the case where xxx _was_ resolved but to more than one value be
> improved in both utility and comprehensibility by providing an error
> message that
>
> 1) indicates that xxx was a valid pathspec, but not a unique one

Just the terminology, you are no longer talking about a pathspec.
You are talking about a rev; i.e. when refs/remotes/origin[12]/xxx 
exist, the user may have meant 'xxx' as a rev, but Git is not allowed
to pick one of them randomly.

It would be nice to take this case into account.

Note that if refs/remotes/origin/xxy exists and the user misspelled
it as 'xxx', you would still get the same "(because 'xxx' cannot be
a rev, it must be a pathspec) pathspec 'xxx' did not match..." error
message, though, so there might not be much point in special casing
"more than one potentially matching revs" case over "there is no
potentially matching revs" case, though.

> 2) provides a list of unique pathspecs that xxx matched against
>
> e.g. in the case where xxx is the name of a branch on both origin1 and
> origin2, it would be ideal if git could instead report
>
>> error: pathspec 'xxx' could not be uniquely resolved
>> xxx can refer to one of the following:
>> * branch origin1/xxx
>> * branch origin2/xxx

Again you are talking about "revs", not pathspecs.  The above (with
tweak to the wrong terminology) would work as a better error message
*if* there is no chance that the user meant 'xxx' as a pathspec,
i.e. "I want to overwrite the files in the working tree that matches
the pathspec 'xxx' with matching contents from the index".

So a possible implementation approach would be

 - to let the current code do what it is doing

 - except that you add new code immediately before the code that
   issues 'xxx' did not match (i.e. the code already checked that
   'xxx' taken as a pathspec does not match anything, and about to
   give the error message but hasn't done so just yet).

 - your new code 

   . checks if 'xxx' could be an attempt to refer to a rev but
 insufficiently spelled out (e.g. both origin[12]/xxx exists, or
 for a bonus point, a similarly named origin/xxy exists and
 could be a typo).

   . if the above check found something, then you report it and
 terminate without complaining "pathspec 'xxx' did not
 match..."

   . on the other hand, if the above check did not find anything,
 then you let the current code issue the same error message as
 before.

Re: [PATCH 1/4] imap-send: add wrapper to get server credentials if needed

[Junio C Hamano]

Jeff King  writes:

> On Mon, Aug 07, 2017 at 09:55:56PM +0200, Nicolas Morey-Chaisemartin wrote:
>
>> > On the other hand, if we're hoping to get rid of this code in favor of
>> > the curl-based approach, then it's not worth spending time on
>> > cosmetic refactoring, as long as it still behaves correctly in the
>> > interim.
>> 
>> Looking at the code, it seems the tunnel mode always uses the legacy imap 
>> approach.
>> This would have to be ported to curl and stabilized before dropping the 
>> legacy code.
>
> Urgh. That's an important mode, I'd think, and one that I have a feeling
> curl may not be interested in supporting, just because of it's
> complexity. And even if they did, it would take a while for that curl
> version to become available.
>
> So maybe the idea of deprecating the non-curl implementation is not
> something that can happen anytime soon. :(
>
>> In the meantime, it might be worth doing a bit of cleanup.
>
> In which case, yeah, it is definitely worth cleaning up the existing
> code. But I also agree with you that it's worth making sure the curl
> version behaves as similarly as possible.

Thanks for guiding this topic forward.  I agree with all points you
raised in your reviews.

Re: What's cooking in git.git (Jul 2017, #09; Mon, 31)

[Igor Djordjevic]

On 07/08/2017 23:25, Igor Djordjevic wrote:
> On 06/08/2017 22:26, Ævar Arnfjörð Bjarmason wrote:
>> On Sat, Aug 05 2017, Junio C. Hamano jotted:
>>> I actually consider "branch" to *never* invoking a checkout.  Even
>>> when "git branch -m A B" happens to be done when your checked out
>>> branch is A and you end up being on B.  That is not a "checkout".
>>
>> I think we just have a different mental model of what "checkout"
>> means. In my mind any operation that updates the HEAD to point to a new
>> branch is a checkout of that branch.
> 
> If I may, from a side-viewer`s point of view, it seems you`re 
> thinking in low-level implementation details, where what Junio 
> describes seems more as a high-level, conceptual/end-user`s point of 
> view.
> 
> Needing to update HEAD reference once we "rename" a branch, too, what 
> you consider a "checkout", seems to be required only because branch 
> name _is_ the branch reference in Git, so we need to update HEAD to 
> point to a new/renamed branch reference -- but it`s still the same 
> branch, conceptually.
> 
> Documentation for "git-checkout" states that it is used to "*Switch 
> branches*...[snip]", and that is not what happens here. 
> Implementation-wise it does because we can`t do it differently at the 
> moment, but in user`s eyes it`s still the same branch, so no switch 
> is made as far as the user is concerned.
> 
> In a different implementation, where branches would have permanent 
> references other than their names, no HEAD update would be needed as 
> the reference would still be the same, no matter the name change, 
> making the `git branch -m` situation clear even from your standpoint, 
> I`d say.
> 
>>> Really from the end-user's point of view that is not a checkout.
>>> The user renamed the branch A and the same conceptual entity, which
>>> is a branch, is now called B.  If that branch was what was checked
>>> out (IOW, if that branch was what would be grown by one commit if
>>> the user did "git commit"), then now that branch's name is B.  It is
>>> natural if you ask "symbolic-ref HEAD" what branch is checked out
>>> after renaming A to B (and A happened to be what was checked out),
>>> the answer chould be B.
>>>
>>> It's like the city you live in changed the name of the street your
>>> house is on.  You do not call movers, you do not do anything, but
>>> your address changes.
>>
>> Yeah I see what you mean, although this analogy rapidly breaks down when
>> you poke at it as shown above. My house (a sha1) can be on any number of
>> streets and new ones can be added/removed all the time without changing
>> where my house is at.
> 
> I may be missing something, but I find the house/address analogy a 
> good one, actually, as I understood that "house" resembles a branch 
> reference HEAD is pointing to, not a sha1.
> 
> Even further, and that might be the point of confusion, "house" seems 
> to be more like a "permanent branch reference" I mentioned above, 
> where your address can change (branch being renamed), but you would 
> still be in the same house (HEAD would still point to the same 
> permanent branch reference).
> 
> If you move to another house, only then would HEAD change to point to 
> another (permanent) branch reference (a different house), and that 
> would be a checkout.
> 
> Yes, it`s not really how it works from the inside, but I think that`s 
> irrelevant for the end-user experience :)
> 
>> So it's just a way to get something exactly like -m except the "move &&
>> checkout" logic is changed to "copy && checkout".
>
> Again, it seems the "checkout" part of "move && checkout" you`re 
> talking about is a user-wise unnecessary implementation detail. For 
> the user, it`s just a simple "move", staying on the same, but renamed 
> branch, thus no branch switching occurred (no "checkout", as per 
> documentation).

All this said, having you mentioning the two argument version:

> $ git checkout master
> $ git branch -m topic avar/topic

... exactly proves the point that "git branch -m" is not expected to 
involve a checkout, even from implementation perspective. It`s just a 
consequence of needing to update the (now obsolete) reference HEAD 
points to (only) when the branch we`re renaming (moving) is the one 
that is currently checked-out.

> Yeah it's not something I'm interested in or have a use-case for,
> although I think in the same way we have -t for checkout it might be
> sensible to have e.g.:
> 
> $ git checkout -b topic-2 -c topic -t origin/master
> 
> Where the new -c or --config-from would mean "...and get the config from
> 'topic'". Such a name would probably be less confusing than
> --super-b[branch?] which to be implies some ongoing hierarchical
> relationship.

This, on the other hand, sounds sensible, staying true to the 
existing logic. In the same manner you can do either:

$ git branch topic -t origin/master
$ git checkout topic

... or shorter equivalent:

$ git checkout -b topic -t 

[PATCH] perl/Git.pm: typofix in a comment

[Junio C Hamano]

No change of behaviour intended.

Signed-off-by: Junio C Hamano 
--
 perl/Git.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index f4b56e6d4d..ffa09ace92 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -532,7 +532,7 @@ If TIME is not supplied, the current local time is used.
 =cut
 
 sub get_tz_offset {
-   # some systmes don't handle or mishandle %z, so be creative.
+   # some systems don't handle or mishandle %z, so be creative.
my $t = shift || time;
my $gm = timegm(localtime($t));
my $sign = qw( + + - )[ $gm <=> $t ];

Re: [PATCH] git svn fetch: Create correct commit timestamp when using --localtime

[Junio C Hamano]

Urs Thuermann  writes:

> In parse_svn_date() prepend the correct UTC offset to the timestamp
> returned.  This is the offset in effect at the commit time instead of
> the offset in effect at calling time.
>
> Signed-off-by: Urs Thuermann 
> ---
>  perl/Git/SVN.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thanks; sounds sensible.  

Eric?

>
> diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
> index 98518f4..bc4eed3 100644
> --- a/perl/Git/SVN.pm
> +++ b/perl/Git/SVN.pm
> @@ -1416,7 +1416,7 @@ sub parse_svn_date {
>   delete $ENV{TZ};
>   }
>  
> - my $our_TZ = get_tz_offset();
> + my $our_TZ = get_tz_offset($epoch_in_UTC);
>  
>   # This converts $epoch_in_UTC into our local timezone.
>   my ($sec, $min, $hour, $mday, $mon, $year,

Suggestion: better error message when an ambiguous checkout is executed

[Mahmoud Al-Qudsi]

Hello,

The default git behavior when attempting to `git checkout xxx` for
some value of "xxx" that cannot be resolved to a single, unique
file/path/branch/tag/commit/etc is to display the following:

> error: pathspec 'xxx' did not match any file(s) known to git

Unfortunately, this is (IMHO) at best misleading when the actual case
is that "git could not unambiguously resolve pathspec xxx"

Can the case where xxx _was_ resolved but to more than one value be
improved in both utility and comprehensibility by providing an error
message that

1) indicates that xxx was a valid pathspec, but not a unique one
2) provides a list of unique pathspecs that xxx matched against

e.g. in the case where xxx is the name of a branch on both origin1 and
origin2, it would be ideal if git could instead report

> error: pathspec 'xxx' could not be uniquely resolved
> xxx can refer to one of the following:
> * branch origin1/xxx
> * branch origin2/xxx

or, less ideally but much simpler, only the first line of that message?

Thank you,

Mahmoud Al-Qudsi
NeoSmart Technologies

Re: [PATCH] Drop some dashes from built-in invocations in scripts

[Junio C Hamano]

Johannes Schindelin  writes:

> I feel a bit talked to my hand, as the only reply I was graced was a "I
> think I already did". So this will be my last reply on this matter for a
> while.

Ah, I meant this thing:

  https://public-inbox.org/git/xmqqo9rrqp3l@gitster.mtv.corp.google.com

I got an impression that you didn't read it before you typed the
message I gave that response to.

There are a few more exchanged, including Michael's response to that
message on that thread.

Re: [GSoC][PATCH 08/13] submodule: port submodule subcommand 'summary' from shell to C

[Christian Couder]

On Mon, Aug 7, 2017 at 11:18 PM, Prathamesh Chavan  wrote:

> +static enum {
> +   DIFF_INDEX,
> +   DIFF_FILES
> +} diff_cmd = DIFF_INDEX;

Using an enum could be a good idea, but I am not sure about using a
static variable.

> +static int compute_summary_module_list(char *head, struct summary_cb *info)
> +{
> +   struct argv_array diff_args = ARGV_ARRAY_INIT;
> +   struct rev_info rev;
> +   struct module_cb_list list = MODULE_CB_LIST_INIT;
> +
> +   argv_array_push(_args, diff_cmd ? "diff-files" : "diff-index");

Maybe diff_cmd could be an argument of compute_summary_module_list()
instead of a static variable, as it's only used in this function and
in module_summary() below which is calling it.

[...]

> +   if (files) {
> +   if (cached)
> +   die(_("The --cached option cannot be used with the 
> --files option"));
> +   diff_cmd++;

Couldn't this be:

   diff_cmd = DIFF_FILES;

?

Re: [suggestion] Include commit-ish in git status output

[Mahmoud Al-Qudsi]

On Fri, Jun 16, 2017 at 4:41 PM, Junio C Hamano  wrote:
>
> HEAD is, unless you are about to create a root commit, always a
> commit and not other kind of commit-ish, so there is no need to say
> "or commit-ish" here.

I apologize for my errant terminology, I thought commitish was what
the abbreviated
SHA1 was called. My proposal was to show either the full SHA1 or the abbreviated
SHA1 in the output of `git status`.

> What would you do differently, after seeing this random-looking
> 40-character string, based on what it is?  Do you know recent commit
> object names by heart and can tell, immediately when you see 88ce3...,
> "ah, that was me fixing foo", as opposed to e140f7a... that is a
> different change you can immediately identify?

I obviously do not know recent commits by heart - the aim is to be
able to easily
copy & paste or visually compare against another value.

Aside from the practical implications of having the commit SHA
included in the output
of `git status`, I have also pointed out my ideological reasons for
it. `git status` currently
prints an incomplete picture of the local repository state, and
without an indication of
_which_ commit HEAD currently is, the remainder of the content
"expires" and is of no
use at some later date.

Looking back, I probably should have started with that. `git status`
gives the status of the
_relative_ current state of the local repository without printing any
information that can be
used as an _absolute_ reference to "frame" the results of the `git
status` command. The
relative state of a repository is useless for any sort of machine or
human analysis, since it
would require the state of both the cached remote and local indices to
be identical to be of
any use.

If I run `git status`, make, commit, and push some changes, then run
`git status` once more,
the output of the command can be identical to the previous run, _even
though the actual
state of the repo has changed_ which is... less than useful and
potentially misleading.

Yes, anyone familiar with git knows that the output of `git status` is
only showing you a summary
of the diff of the working tree vs HEAD. My argument is that all it
would take is another 8 characters
appending to the "On branch " line to make it an infinitely more
useful command.

Thank you,

Mahmoud Al-Qudsi
NeoSmart Technologies

Re: [PATCH v4 4/4] add: modify already added files when --chmod is given

[René Scharfe]

Am 14.09.2016 um 23:07 schrieb Thomas Gummerer:
> When the chmod option was added to git add, it was hooked up to the diff
> machinery, meaning that it only works when the version in the index
> differs from the version on disk.
> 
> As the option was supposed to mirror the chmod option in update-index,
> which always changes the mode in the index, regardless of the status of
> the file, make sure the option behaves the same way in git add.
> 
> Signed-off-by: Thomas Gummerer 

Sorry for replying almost a year late, hopefully you're still interested.

> ---
>   builtin/add.c  | 47 ---
>   builtin/checkout.c |  2 +-
>   builtin/commit.c   |  2 +-
>   cache.h| 10 +-
>   read-cache.c   | 14 ++
>   t/t3700-add.sh | 50 ++
>   6 files changed, 91 insertions(+), 34 deletions(-)
> 
> diff --git a/builtin/add.c b/builtin/add.c
> index b1dddb4..595a0b2 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -26,10 +26,25 @@ static int patch_interactive, add_interactive, 
> edit_interactive;
>   static int take_worktree_changes;
>   
>   struct update_callback_data {
> - int flags, force_mode;
> + int flags;
>   int add_errors;
>   };
>   
> +static void chmod_pathspec(struct pathspec *pathspec, int force_mode)

"int force_mode" looks like a binary (or perhaps ternary) flag, but
actually it is a character and can only have the values '-' or '+'.
In builtin/update-index.c it's called "char flip" and we probably should
define it like this here as well.

> +{
> + int i;
> + 
> + for (i = 0; i < active_nr; i++) {
> + struct cache_entry *ce = active_cache[i];
> +
> + if (pathspec && !ce_path_match(ce, pathspec, NULL))
> + continue;
> +
> + if (chmod_cache_entry(ce, force_mode) < 0)
> + fprintf(stderr, "cannot chmod '%s'", ce->name);

This error message is missing a newline.  In builtin/update-index.c we
also show the attempted change (-x or +x); perhaps we want to do that
here as well.

Currently chmod_cache_entry() can only fail if ce is not a regular
file or it's other parameter is neither '-' nor '+'.  We rule out the
latter already in the argument parsing code.  The former can happen if
we add a symlink, either explicitly or because it's in a directory
we're specified.

I wonder if we even need to report anything, or under which conditions.
If you have a file named dir/file and a symlink named dir/symlink then
the interesting cases are:

git add --chmod=.. dir/symlink
git add --chmod=.. dir/file dir/symlink
git add --chmod=.. dir

Warning about each case may be the most cautious thing to do, but
documenting that --chmod has no effect on symlinks and keeping silent
might be less annoying, especially in the last case.  What do you
think?

> @@ -342,13 +354,8 @@ int cmd_add(int argc, const char **argv, const char 
> *prefix)
>   if (!show_only && ignore_missing)
>   die(_("Option --ignore-missing can only be used together with 
> --dry-run"));
>   
> - if (!chmod_arg)
> - force_mode = 0;
> - else if (!strcmp(chmod_arg, "-x"))
> - force_mode = 0666;
> - else if (!strcmp(chmod_arg, "+x"))
> - force_mode = 0777;
> - else
> + if (chmod_arg && ((chmod_arg[0] != '-' && chmod_arg[0] != '+') ||
> +   chmod_arg[1] != 'x' || chmod_arg[2]))
>   die(_("--chmod param '%s' must be either -x or +x"), chmod_arg);

That's the argument parsing code mentioned above.  The strcmp-based
checks look nicer to me btw.  How about this?

if (chmod_arg && strcmp(chmod_arg, "-x") && strcmp(chmod_arg, "+x"))

But that's just nitpicking.

René

Re: [PATCH] Fix delta integer overflows

[Junio C Hamano]

Johannes Schindelin  writes:

> So are you saying that starting with v2.14.0, you accept patches into `pu`
> for which you would previously have required multiple iterations before
> even considering it for `pu`?
>
> Frankly, I am a bit surprised that this obvious change from `unsigned
> long` to `size_t` is not required in this case before queuing, but if the
> rules have changed to lower the bar for patch submissions, I am all for
> it. I always felt that we are wasting contributors' time a little too
> freely and too deliberately.

This is not about where the bar is set.  It is about expectation.  I
do not expect much from occasional (or new) contributors and I try
not to demand too much from them.  The consequence is that as long
as a small patch makes things better without making anything worse,
I'd want to be inclusive to encourage them to build obvious
improvements on top.  Maybe they just want a single patch landed to
fix their immediate needs (which may be generic enough and expected
to be shared with many other people) without going further, so I may
end up queuing something that only helps 40% of people until follow
up patches are done to cover the remaining 60% of people, but that
is fine as long as the patch does not make things worse (it is not
like a patch that helps 40% while hurting the remaining 60% until
a follow-up happens).

I would expect a lot more from experienced contributors, when I know
they are capable of helping the remaining 60% inside the same series
and without requiring too much hand-holding from me.  The same thing
I cannot say to a occasional (or new) contributor---they may not be
coorperative, or they may be willing to do more but may require more
help polishing their work than the bandwidth I can afford.

So if you are volunteering to help by either guiding Martin to aim
higher and make this a series with a larger and more complete scope,
I'd very much appreciate it.  Or you can do a follow-up series on
top of what Martin posted.  Either is fine by me.  Just do not step
on each others' toes ;-)

I avoided saying all of the above because I didn't want my word
taken out of context later to make it sound as if I were belittling
the competence of Martin, but you seem to want to force me to say
this, so here it is.

Re: What's cooking in git.git (Jul 2017, #09; Mon, 31)

[Igor Djordjevic]

On 06/08/2017 22:26, Ævar Arnfjörð Bjarmason wrote:
> On Sat, Aug 05 2017, Junio C. Hamano jotted:
>> I actually consider "branch" to *never* invoking a checkout.  Even
>> when "git branch -m A B" happens to be done when your checked out
>> branch is A and you end up being on B.  That is not a "checkout".
> 
> I think we just have a different mental model of what "checkout"
> means. In my mind any operation that updates the HEAD to point to a new
> branch is a checkout of that branch.

If I may, from a side-viewer`s point of view, it seems you`re 
thinking in low-level implementation details, where what Junio 
describes seems more as a high-level, conceptual/end-user`s point of 
view.

Needing to update HEAD reference once we "rename" a branch, too, what 
you consider a "checkout", seems to be required only because branch 
name _is_ the branch reference in Git, so we need to update HEAD to 
point to a new/renamed branch reference -- but it`s still the same 
branch, conceptually.

Documentation for "git-checkout" states that it is used to "*Switch 
branches*...[snip]", and that is not what happens here. 
Implementation-wise it does because we can`t do it differently at the 
moment, but in user`s eyes it`s still the same branch, so no switch 
is made as far as the user is concerned.

In a different implementation, where branches would have permanent 
references other than their names, no HEAD update would be needed as 
the reference would still be the same, no matter the name change, 
making the `git branch -m` situation clear even from your standpoint, 
I`d say.

>> Really from the end-user's point of view that is not a checkout.
>> The user renamed the branch A and the same conceptual entity, which
>> is a branch, is now called B.  If that branch was what was checked
>> out (IOW, if that branch was what would be grown by one commit if
>> the user did "git commit"), then now that branch's name is B.  It is
>> natural if you ask "symbolic-ref HEAD" what branch is checked out
>> after renaming A to B (and A happened to be what was checked out),
>> the answer chould be B.
>>
>> It's like the city you live in changed the name of the street your
>> house is on.  You do not call movers, you do not do anything, but
>> your address changes.
> 
> Yeah I see what you mean, although this analogy rapidly breaks down when
> you poke at it as shown above. My house (a sha1) can be on any number of
> streets and new ones can be added/removed all the time without changing
> where my house is at.

I may be missing something, but I find the house/address analogy a 
good one, actually, as I understood that "house" resembles a branch 
reference HEAD is pointing to, not a sha1.

Even further, and that might be the point of confusion, "house" seems 
to be more like a "permanent branch reference" I mentioned above, 
where your address can change (branch being renamed), but you would 
still be in the same house (HEAD would still point to the same 
permanent branch reference).

If you move to another house, only then would HEAD change to point to 
another (permanent) branch reference (a different house), and that 
would be a checkout.

Yes, it`s not really how it works from the inside, but I think that`s 
irrelevant for the end-user experience :)

> So it's just a way to get something exactly like -m except the "move &&
> checkout" logic is changed to "copy && checkout".
Again, it seems the "checkout" part of "move && checkout" you`re 
talking about is a user-wise unnecessary implementation detail. For 
the user, it`s just a simple "move", staying on the same, but renamed 
branch, thus no branch switching occurred (no "checkout", as per 
documentation).

Regards,
Buga

[GSoC][PATCH 13/13] submodule: port submodule subcommand 'foreach' from shell to C

[Prathamesh Chavan]

This aims to make git-submodule foreach a builtin. This is the very
first step taken in this direction. Hence, 'foreach' is ported to
submodule--helper, and submodule--helper is called from git-submodule.sh.
The code is split up to have one function to obtain all the list of
submodules. This function acts as the front-end of git-submodule foreach
subcommand. It calls the function for_each_submodule_list, which basically
loops through the list and calls function fn, which in this case is
runcommand_in_submodule. This third function is a calling function that
takes care of running the command in that submodule, and recursively
perform the same when --recursive is flagged.

The first function module_foreach first parses the options present in
argv, and then with the help of module_list_compute, generates the list of
submodules present in the current working tree.

The second function for_each_submodule_list traverses through the
list, and calls function fn (which in case of submodule subcommand
foreach is runcommand_in_submodule) is called for each entry.

The third function runcommand_in_submodule, generates a submodule struct sub
for $name, value and then later prepends name=sub->name; and other
value assignment to the env argv_array structure of a child_process.
Also the  of submodule-foreach is push to args argv_array
structure and finally, using run_command the commands are executed
using a shell.

The third function also takes care of the recursive flag, by creating
a separate child_process structure and prepending "--super-prefix displaypath",
to the args argv_array structure. Other required arguments and the
input  of submodule-foreach is also appended to this argv_array.

Helped-by: Brandon Williams 
Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
In this new version, the following changes have been made:
* A comment was added to clarify why the env variables were made
  available only for the case of argc == 1.

 builtin/submodule--helper.c | 142 
 git-submodule.sh|  39 +---
 2 files changed, 143 insertions(+), 38 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 2080f4fb9..0717ecf80 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -770,6 +770,147 @@ static int module_name(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
+struct cb_foreach {
+   int argc;
+   const char **argv;
+   const char *prefix;
+   unsigned int quiet: 1;
+   unsigned int recursive: 1;
+};
+#define CB_FOREACH_INIT { 0, NULL, NULL, 0, 0 }
+
+static void runcommand_in_submodule(const struct cache_entry *list_item,
+   void *cb_data)
+{
+   struct cb_foreach *info = cb_data;
+   const struct submodule *sub;
+   struct child_process cp = CHILD_PROCESS_INIT;
+   char *displaypath;
+
+   displaypath = get_submodule_displaypath(list_item->name, info->prefix);
+
+   sub = submodule_from_path(null_sha1, list_item->name);
+
+   if (!sub)
+   die(_("No url found for submodule path '%s' in .gitmodules"),
+ displaypath);
+
+   if (!is_submodule_populated_gently(list_item->name, NULL))
+   goto cleanup;
+
+   prepare_submodule_repo_env(_array);
+
+   /*
+* For the purpose of executing  in the submodule,
+* separate shell is used for the purpose of running the
+* child process.
+*/
+   cp.use_shell = 1;
+   cp.dir = list_item->name;
+
+   /*
+* NEEDSWORK: the command currently has access to the variables $name,
+* $sm_path, $displaypath, $sha1 and $toplevel only when the command
+* contains a single argument. This is done for maintianing a faithful
+* translation from shell script.
+*/
+   if (info->argc == 1) {
+   char *toplevel = xgetcwd();
+
+   argv_array_pushf(_array, "name=%s", sub->name);
+   argv_array_pushf(_array, "sm_path=%s", list_item->name);
+   argv_array_pushf(_array, "displaypath=%s", displaypath);
+   argv_array_pushf(_array, "sha1=%s",
+oid_to_hex(_item->oid));
+   argv_array_pushf(_array, "toplevel=%s", toplevel);
+
+   /*
+* Since the path variable was accessible from the script
+* before porting, it is also made available after porting.
+* The environment variable "PATH" has a very special purpose
+* on windows. And since environment variables are
+* case-insensitive in windows, it interferes with the
+* existing PATH variable. Hence, to avoid that, we expose
+* path via the args 

[GSoC][PATCH 12/13] submodule foreach: document variable '$displaypath'

[Prathamesh Chavan]

It was observed that the variable '$displaypath' was accessible but
undocumented. Hence, document it.

Discussed-with: Ramsay Jones 
Signed-off-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 Documentation/git-submodule.txt |  6 --
 t/t7407-submodule-foreach.sh| 22 +++---
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 8e7930ebc..0cca702cb 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -183,10 +183,12 @@ information too.
 
 foreach [--recursive] ::
Evaluates an arbitrary shell command in each checked out submodule.
-   The command has access to the variables $name, $sm_path, $sha1 and
-   $toplevel:
+   The command has access to the variables $name, $sm_path, $displaypath,
+   $sha1 and $toplevel:
$name is the name of the relevant submodule section in `.gitmodules`,
$sm_path is the path of the submodule as recorded in the superproject,
+   $displaypath contains the relative path from the current working
+   directory to the submodules root directory,
$sha1 is the commit as recorded in the superproject, and
$toplevel is the absolute path to its superproject, such that
$toplevel/$sm_path is the absolute path of the submodule.
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 0663622a4..6ad57e061 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -82,16 +82,16 @@ test_expect_success 'test basic "submodule foreach" usage' '
 
 cat >expect <../../actual
+   git submodule foreach "echo 
\$toplevel-\$name-\$sm_path-\$displaypath-\$sha1" >../../actual
) &&
test_i18ncmp expect actual
 '
@@ -206,25 +206,25 @@ submodulesha1=$(cd 
clone2/nested1/nested2/nested3/submodule && git rev-parse HEA
 
 cat >expect <../../actual
+   git submodule foreach --recursive "echo 
\$toplevel-\$name-\$sm_path-\$displaypath-\$sha1" >../../actual
) &&
test_i18ncmp expect actual
 '
-- 
2.13.0

[GSoC][PATCH 08/13] submodule: port submodule subcommand 'summary' from shell to C

[Prathamesh Chavan]

The submodule subcommand 'summary' is ported in the process of
making git-submodule a builtin. The function cmd_summary() from
git-submodule.sh is ported to functions module_summary(),
compute_summary_module_list(), prepare_submodule_summary() and
generate_submodule_summary(), print_summary().

The first function module_summary() parses the options of submodule
subcommand and also acts as the front-end of this subcommand.
After parsing them, it calls the compute_summary_module_list()

The functions compute_summary_module_list() runs the diff_cmd,
and generates the modules list, as required by the subcommand.
The generation of this module list is done by the using the
callback function submodule_summary_callback(), and stored in the
structure module_cb.

Once the module list is generated, prepare_submodule_summary()
further goes through the list and filters the list, for
eventually calling the generate_submodule_summary() function.

The function generate_submodule_summary() takes care of generating
the summary for each submodule and then calls the function
print_summary() for printing it.

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
The major changes the patch underwent through is the splitting of the
function print_submodule_summary() into generate_submodule_summary()
and print_summary()

Apart from this, there are also minor changes which include
removal of variables sha1_abbr_dst and sha1_abbr_src,
the variable remote_key wasn't freed earlier, but now it is.

 builtin/submodule--helper.c | 428 
 git-submodule.sh| 183 +--
 2 files changed, 429 insertions(+), 182 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 02787e4a4..2080f4fb9 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -13,6 +13,9 @@
 #include "remote.h"
 #include "refs.h"
 #include "connect.h"
+#include "revision.h"
+#include "diffcore.h"
+#include "diff.h"
 
 typedef void (*submodule_list_func_t)(const struct cache_entry *list_item,
  void *cb_data);
@@ -767,6 +770,430 @@ static int module_name(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
+struct module_cb {
+   unsigned int mod_src;
+   unsigned int mod_dst;
+   struct object_id oid_src;
+   struct object_id oid_dst;
+   char status;
+   const char *sm_path;
+};
+#define MODULE_CB_INIT { 0, 0, NULL, NULL, '\0', NULL }
+
+struct module_cb_list {
+   struct module_cb **entries;
+   int alloc, nr;
+};
+#define MODULE_CB_LIST_INIT { NULL, 0, 0 }
+
+struct summary_cb {
+   int argc;
+   const char **argv;
+   const char *prefix;
+   unsigned int cached: 1;
+   unsigned int for_status: 1;
+   unsigned int quiet: 1;
+   unsigned int files: 1;
+   int summary_limits;
+};
+#define SUMMARY_CB_INIT { 0, NULL, NULL, 0, 0, 0, 0, 0 }
+
+static enum {
+   DIFF_INDEX,
+   DIFF_FILES
+} diff_cmd = DIFF_INDEX;
+
+static int verify_submodule_object_name(const char *sm_path, const char *sha1)
+{
+   struct child_process cp_rev_parse = CHILD_PROCESS_INIT;
+
+   cp_rev_parse.git_cmd = 1;
+   cp_rev_parse.no_stdout = 1;
+   cp_rev_parse.dir = sm_path;
+   prepare_submodule_repo_env(_rev_parse.env_array);
+   argv_array_pushl(_rev_parse.args, "rev-parse", "-q",
+"--verify", NULL);
+   argv_array_pushf(_rev_parse.args, "%s^0", sha1);
+
+   if (run_command(_rev_parse))
+   return 1;
+
+   return 0;
+}
+
+static void print_summary(struct summary_cb *info, int errmsg, int 
total_commits,
+ int missing_src, int missing_dst, const char 
*displaypath,
+ int is_sm_git_dir, struct module_cb *p)
+{
+   if (p->status == 'T') {
+   if (S_ISGITLINK(p->mod_dst))
+   printf(_("* %s %s(blob)->%s(submodule)"),
+displaypath, 
find_unique_abbrev(p->oid_src.hash, 7),
+find_unique_abbrev(p->oid_dst.hash, 7));
+   else
+   printf(_("* %s %s(submodule)->%s(blob)"),
+displaypath, 
find_unique_abbrev(p->oid_src.hash, 7),
+find_unique_abbrev(p->oid_dst.hash, 7));
+   } else {
+   printf("* %s %s...%s", displaypath, 
find_unique_abbrev(p->oid_src.hash, 7),
+find_unique_abbrev(p->oid_dst.hash, 7));
+   }
+
+   if (total_commits < 0)
+   printf(":\n");
+   else
+   printf(" (%d):\n", total_commits);
+
+   if (errmsg) {
+   /*
+* Don't give error msg for modification whose dst is not
+* submodule, i.e. 

[GSoC][PATCH 10/13] submodule foreach: document '$sm_path' instead of '$path'

[Prathamesh Chavan]

As using a variable '$path' may be harmful to users due to
capitalization issues, see 64394e3ae9 (git-submodule.sh: Don't
use $path variable in eval_gettext string, 2012-04-17). Adjust
the documentation to advocate for using $sm_path,  which contains
the same value. We still make the 'path' variable available and
document it as a deprecated synonym of 'sm_path'.

Discussed-with: Ramsay Jones 
Signed-off-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 Documentation/git-submodule.txt | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index ff612001d..a23baef62 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -183,12 +183,14 @@ information too.
 
 foreach [--recursive] ::
Evaluates an arbitrary shell command in each checked out submodule.
-   The command has access to the variables $name, $path, $sha1 and
+   The command has access to the variables $name, $sm_path, $sha1 and
$toplevel:
$name is the name of the relevant submodule section in `.gitmodules`,
-   $path is the name of the submodule directory relative to the
-   superproject, $sha1 is the commit as recorded in the superproject,
-   and $toplevel is the absolute path to the top-level of the superproject.
+   $sm_path is the path of the submodule as recorded in the superproject,
+   $sha1 is the commit as recorded in the superproject, and
+   $toplevel is the absolute path to the top-level of the superproject.
+   Note that to avoid conflicts with '$PATH' on Windows, the '$path'
+   variable is now a deprecated synonym of '$sm_path' variable.
Any submodules defined in the superproject but not checked out are
ignored by this command. Unless given `--quiet`, foreach prints the name
of each submodule before evaluating the command.
-- 
2.13.0

[GSoC][PATCH 11/13] submodule foreach: clarify the '$toplevel' variable documentation

[Prathamesh Chavan]

It does not contain the topmost superproject as the author assumed,
but the direct superproject, such that $toplevel/$sm_path is the
actual absolute path of the submodule.

Discussed-with: Ramsay Jones 
Signed-off-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 Documentation/git-submodule.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index a23baef62..8e7930ebc 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -188,7 +188,8 @@ foreach [--recursive] ::
$name is the name of the relevant submodule section in `.gitmodules`,
$sm_path is the path of the submodule as recorded in the superproject,
$sha1 is the commit as recorded in the superproject, and
-   $toplevel is the absolute path to the top-level of the superproject.
+   $toplevel is the absolute path to its superproject, such that
+   $toplevel/$sm_path is the absolute path of the submodule.
Note that to avoid conflicts with '$PATH' on Windows, the '$path'
variable is now a deprecated synonym of '$sm_path' variable.
Any submodules defined in the superproject but not checked out are
-- 
2.13.0

[GSoC][PATCH 09/13] submodule foreach: correct '$path' in nested submodules from a subdirectory

[Prathamesh Chavan]

When running 'git submodule foreach' from a subdirectory of your
repository, nested submodules get a bogus value for $sm_path:
For a submodule 'sub' that contains a nested submodule 'nested',
running 'git -C dir submodule foreach echo $path' would report
path='../nested' for the nested submodule. The first part '../' is
derived from the logic computing the relative path from $pwd to the
root of the superproject. The second part is the submodule path inside
the submodule. This value is of little use and is hard to document.

There are two different possible solutions that have more value:
(a) The path value is documented as the path from the toplevel of the
superproject to the mount point of the submodule.
In this case we would want to have path='sub/nested'.

(b) As Ramsay noticed the documented value is wrong. For the non-nested
case the path is equal to the relative path from $pwd to the
submodules working directory. When following this model,
the expected value would be path='../sub/nested'.

The behavior for (b) was introduced in 091a6eb0fe (submodule: drop the
top-level requirement, 2013-06-16) the intent for $path seemed to be
relative to $cwd to the submodule worktree, but that did not work for
nested submodules, as the intermittent submodules were not included in
the path.

If we were to fix the meaning of the $path using (a) such that "path"
is "the path from the toplevel of the superproject to the mount point
of the submodule", we would break any existing submodule user that runs
foreach from non-root of the superproject as the non-nested submodule
'../sub' would change its path to 'sub'.

If we would fix the meaning of the $path using (b), such that "path"
is "the relative path from $pwd to the submodule", then we would break
any user that uses nested submodules (even from the root directory) as
the 'nested' would become 'sub/nested'.

Both groups can be found in the wild.  The author has no data if one group
outweighs the other by large margin, and offending each one seems equally
bad at first.  However in the authors imagination it is better to go with
(a) as running from a sub directory sounds like it is carried out
by a human rather than by some automation task.  With a human on
the keyboard the feedback loop is short and the changed behavior can be
adapted to quickly unlike some automation that can break silently.

Discussed-with: Ramsay Jones 
Signed-off-by: Prathamesh Chavan 
Signed-off-by: Stefan Beller 
---
 git-submodule.sh |  1 -
 t/t7407-submodule-foreach.sh | 36 ++--
 2 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index ec57d6528..4b7da2fc1 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -320,7 +320,6 @@ cmd_foreach()
prefix="$prefix$sm_path/"
sanitize_submodule_env
cd "$sm_path" &&
-   sm_path=$(git submodule--helper relative-path 
"$sm_path" "$wt_prefix") &&
# we make $path available to scripts ...
path=$sm_path &&
if test $# -eq 1
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 6ba5daf42..0663622a4 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -82,9 +82,9 @@ test_expect_success 'test basic "submodule foreach" usage' '
 
 cat >expect  expect <

[GSoC][PATCH 07/13] diff: change scope of the function count_lines()

[Prathamesh Chavan]

Change the scope of function count_lines for allowing the function
to be reused in other parts of the code as well.

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 diff.c | 2 +-
 diff.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index 85e714f6c..03ed64f93 100644
--- a/diff.c
+++ b/diff.c
@@ -425,7 +425,7 @@ struct emit_callback {
struct strbuf *header;
 };
 
-static int count_lines(const char *data, int size)
+int count_lines(const char *data, int size)
 {
int count, ch, completely_empty = 1, nl_just_seen = 0;
count = 0;
diff --git a/diff.h b/diff.h
index 2d442e296..8522514e9 100644
--- a/diff.h
+++ b/diff.h
@@ -273,6 +273,7 @@ extern struct diff_filepair *diff_unmerge(struct 
diff_options *, const char *pat
 extern int parse_long_opt(const char *opt, const char **argv,
 const char **optarg);
 
+extern int count_lines(const char *data, int size);
 extern int git_diff_basic_config(const char *var, const char *value, void *cb);
 extern int git_diff_heuristic_config(const char *var, const char *value, void 
*cb);
 extern void init_diff_ui_defaults(void);
-- 
2.13.0

[GSoC][PATCH 06/13] submodule: port submodule subcommand 'deinit' from shell to C

[Prathamesh Chavan]

The same mechanism is used even for porting this submodule
subcommand, as used in the ported subcommands till now.
The function cmd_deinit in split up after porting into three
functions: module_deinit(), for_each_submodule_list() and
deinit_submodule().

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 builtin/submodule--helper.c | 148 
 git-submodule.sh|  55 +---
 2 files changed, 149 insertions(+), 54 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 82f1aed87..02787e4a4 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -915,6 +915,153 @@ static int module_sync(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
+struct deinit_cb {
+   const char *prefix;
+   unsigned int quiet: 1;
+   unsigned int force: 1;
+   unsigned int all: 1;
+};
+#define DEINIT_CB_INIT { NULL, 0, 0, 0 }
+
+static void deinit_submodule(const struct cache_entry *list_item,
+void *cb_data)
+{
+   struct deinit_cb *info = cb_data;
+   const struct submodule *sub;
+   char *displaypath = NULL;
+   struct child_process cp_config = CHILD_PROCESS_INIT;
+   struct strbuf sb_config = STRBUF_INIT;
+   char *sub_git_dir = xstrfmt("%s/.git", list_item->name);
+   mode_t mode = 0777;
+
+   sub = submodule_from_path(null_sha1, list_item->name);
+
+   if (!sub || !sub->name)
+   goto cleanup;
+
+   displaypath = get_submodule_displaypath(list_item->name, info->prefix);
+
+   /* remove the submodule work tree (unless the user already did it) */
+   if (is_directory(list_item->name)) {
+   struct stat st;
+   /*
+* protect submodules containing a .git directory
+* NEEDSWORK: automatically call absorbgitdirs before
+* warning/die.
+*/
+   if (is_directory(sub_git_dir))
+   die(_("Submodule work tree '%s' contains a .git "
+ "directory use 'rm -rf' if you really want "
+ "to remove it including all of its history"),
+ displaypath);
+
+   if (!info->force) {
+   struct child_process cp_rm = CHILD_PROCESS_INIT;
+   cp_rm.git_cmd = 1;
+   argv_array_pushl(_rm.args, "rm", "-qn",
+list_item->name, NULL);
+
+   if (run_command(_rm))
+   die(_("Submodule work tree '%s' contains local "
+ "modifications; use '-f' to discard 
them"),
+ displaypath);
+   }
+
+   if (!lstat(list_item->name, )) {
+   struct strbuf sb_rm = STRBUF_INIT;
+   const char *format;
+
+   strbuf_addstr(_rm, list_item->name);
+
+   if (!remove_dir_recursively(_rm, 0))
+   format = _("Cleared directory '%s'\n");
+   else
+   format = _("Could not remove submodule work 
tree '%s'\n");
+
+   if (!info->quiet)
+   printf(format, displaypath);
+
+   mode = st.st_mode;
+
+   strbuf_release(_rm);
+   }
+   }
+
+   if (mkdir(list_item->name, mode))
+   die_errno(_("could not create empty submodule directory %s"),
+ displaypath);
+
+   cp_config.git_cmd = 1;
+   argv_array_pushl(_config.args, "config", "--get-regexp", NULL);
+   argv_array_pushf(_config.args, "submodule.%s\\.", sub->name);
+
+   /* remove the .git/config entries (unless the user already did it) */
+   if (!capture_command(_config, _config, 0) && sb_config.len) {
+   char *sub_key = xstrfmt("submodule.%s", sub->name);
+   /*
+* remove the whole section so we have a clean state when
+* the user later decides to init this submodule again
+*/
+   git_config_rename_section_in_file(NULL, sub_key, NULL);
+   if (!info->quiet)
+   printf(_("Submodule '%s' (%s) unregistered for path 
'%s'\n"),
+sub->name, sub->url, displaypath);
+   free(sub_key);
+   }
+
+cleanup:
+   free(displaypath);
+   free(sub_git_dir);
+   strbuf_release(_config);
+}
+
+static int module_deinit(int argc, const char **argv, const char *prefix)
+{
+   struct deinit_cb info = DEINIT_CB_INIT;
+   struct pathspec pathspec;
+   struct module_list list 

[GSoC][PATCH 04/13] submodule: port submodule subcommand 'status' from shell to C

[Prathamesh Chavan]

This aims to make git-submodule 'status' a built-in. Hence, the function
cmd_status() is ported from shell to C. This is done by introducing
three functions: module_status(), submodule_status() and print_status().

The function module_status() acts as the front-end of the subcommand.
It parses subcommand's options and then calls the function
module_list_compute() for computing the list of submodules. Then
this functions calls for_each_submodule_list() looping through the
list obtained.

Then for_each_submodule_list() calls submodule_status() for each of the
submodule in its list. The function submodule_status() is responsible
for generating the status each submodule it is called for, and
then calls print_status().

Finally, the function print_status() handles the printing of submodule's
status.

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 builtin/submodule--helper.c | 156 
 git-submodule.sh|  49 +-
 2 files changed, 157 insertions(+), 48 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 421eee1e2..1bf7bb2a2 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -560,6 +560,161 @@ static int module_init(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
+struct status_cb {
+   const char *prefix;
+   unsigned int quiet: 1;
+   unsigned int recursive: 1;
+   unsigned int cached: 1;
+};
+#define STATUS_CB_INIT { NULL, 0, 0, 0 }
+
+static void print_status(struct status_cb *info, char state, const char *path,
+const struct object_id *oid, const char *displaypath)
+{
+   if (info->quiet)
+   return;
+
+   printf("%c%s %s", state, oid_to_hex(oid), displaypath);
+
+   if (state == ' ' || state == '+') {
+   struct argv_array name_rev_args = ARGV_ARRAY_INIT;
+
+   argv_array_pushl(_rev_args, "print-name-rev",
+path, oid_to_hex(oid), NULL);
+   print_name_rev(name_rev_args.argc, name_rev_args.argv,
+  info->prefix);
+
+   argv_array_clear(_rev_args);
+   } else {
+   printf("\n");
+   }
+}
+
+static int handle_submodule_head_ref(const char *refname,
+const struct object_id *oid, int flags,
+void *cb_data)
+{
+   struct object_id *output = cb_data;
+   if (oid)
+   oidcpy(output, oid);
+
+   return 0;
+}
+
+static void status_submodule(const struct cache_entry *list_item, void 
*cb_data)
+{
+   struct status_cb *info = cb_data;
+   char *displaypath;
+   struct argv_array diff_files_args = ARGV_ARRAY_INIT;
+
+   if (!submodule_from_path(null_sha1, list_item->name))
+   die(_("no submodule mapping found in .gitmodules for path 
'%s'"),
+ list_item->name);
+
+   displaypath = get_submodule_displaypath(list_item->name, info->prefix);
+
+   if (list_item->ce_flags) {
+   print_status(info, 'U', list_item->name,
+_oid, displaypath);
+   goto cleanup;
+   }
+
+   if (!is_submodule_active(the_repository, list_item->name)) {
+   print_status(info, '-', list_item->name, _item->oid,
+displaypath);
+   goto cleanup;
+   }
+
+   argv_array_pushl(_files_args, "diff-files",
+"--ignore-submodules=dirty", "--quiet", "--",
+list_item->name, NULL);
+
+   if (!cmd_diff_files(diff_files_args.argc, diff_files_args.argv,
+   info->prefix)) {
+   print_status(info, ' ', list_item->name, _item->oid,
+displaypath);
+   } else {
+   if (!info->cached) {
+   struct object_id oid;
+
+   if (head_ref_submodule(list_item->name,
+  handle_submodule_head_ref, ))
+   die(_("could not resolve HEAD ref inside the"
+ "submodule '%s'"), list_item->name);
+
+   print_status(info, '+', list_item->name, ,
+displaypath);
+   } else {
+   print_status(info, '+', list_item->name,
+_item->oid, displaypath);
+   }
+   }
+
+   if (info->recursive) {
+   struct child_process cpr = CHILD_PROCESS_INIT;
+
+   cpr.git_cmd = 1;
+   cpr.dir = list_item->name;
+   prepare_submodule_repo_env(_array);
+
+   argv_array_pushl(, "--super-prefix", displaypath,
+  

[GSoC][PATCH 05/13] submodule: port submodule subcommand 'sync' from shell to C

[Prathamesh Chavan]

Port the submodule subcommand 'sync' from shell to C using the same
mechanism as that used for porting submodule subcommand 'status'.
Hence, here the function cmd_sync() is ported from shell to C.
This is done by introducing three functions: module_sync(),
sync_submodule() and print_default_remote().

The function print_default_remote() is introduced for getting
the default remote as stdout.

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 builtin/submodule--helper.c | 183 
 git-submodule.sh|  57 +-
 2 files changed, 184 insertions(+), 56 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 1bf7bb2a2..82f1aed87 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -44,6 +44,20 @@ static char *get_default_remote(void)
return ret;
 }
 
+static int print_default_remote(int argc, const char **argv, const char 
*prefix)
+{
+   const char *remote;
+
+   if (argc != 1)
+   die(_("submodule--helper print-default-remote takes no 
arguments"));
+
+   remote = get_default_remote();
+   if (remote)
+   printf("%s\n", remote);
+
+   return 0;
+}
+
 static int starts_with_dot_slash(const char *str)
 {
return str[0] == '.' && is_dir_sep(str[1]);
@@ -379,6 +393,25 @@ static void module_list_active(struct module_list *list)
*list = active_modules;
 }
 
+static char *get_up_path(const char *path)
+{
+   int i;
+   struct strbuf sb = STRBUF_INIT;
+
+   for (i = count_slashes(path); i; i--)
+   strbuf_addstr(, "../");
+
+   /*
+* Check if 'path' ends with slash or not
+* for having the same output for dir/sub_dir
+* and dir/sub_dir/
+*/
+   if (!is_dir_sep(path[strlen(path) - 1]))
+   strbuf_addstr(, "../");
+
+   return strbuf_detach(, NULL);
+}
+
 static int module_list(int argc, const char **argv, const char *prefix)
 {
int i;
@@ -734,6 +767,154 @@ static int module_name(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
+struct sync_cb {
+   const char *prefix;
+   unsigned int quiet: 1;
+   unsigned int recursive: 1;
+};
+#define SYNC_CB_INIT { NULL, 0, 0 }
+
+static void sync_submodule(const struct cache_entry *list_item, void *cb_data)
+{
+   struct sync_cb *info = cb_data;
+   const struct submodule *sub;
+   char *remote_key = NULL;
+   char *sub_origin_url, *super_config_url, *displaypath;
+   struct strbuf sb = STRBUF_INIT;
+   struct child_process cp = CHILD_PROCESS_INIT;
+   char *sub_config_path = NULL;
+
+   if (!is_submodule_active(the_repository, list_item->name))
+   return;
+
+   sub = submodule_from_path(null_sha1, list_item->name);
+
+   if (sub && sub->url) {
+   if (starts_with_dot_dot_slash(sub->url) || 
starts_with_dot_slash(sub->url)) {
+   char *remote_url, *up_path;
+   char *remote = get_default_remote();
+   strbuf_addf(, "remote.%s.url", remote);
+
+   if (git_config_get_string(sb.buf, _url))
+   remote_url = xgetcwd();
+
+   up_path = get_up_path(list_item->name);
+   sub_origin_url = relative_url(remote_url, sub->url, 
up_path);
+   super_config_url = relative_url(remote_url, sub->url, 
NULL);
+
+   free(remote);
+   free(up_path);
+   free(remote_url);
+   } else {
+   sub_origin_url = xstrdup(sub->url);
+   super_config_url = xstrdup(sub->url);
+   }
+   } else {
+   sub_origin_url = "";
+   super_config_url = "";
+   }
+
+   displaypath = get_submodule_displaypath(list_item->name, info->prefix);
+
+   if (!info->quiet)
+   printf(_("Synchronizing submodule url for '%s'\n"),
+displaypath);
+
+   strbuf_reset();
+   strbuf_addf(, "submodule.%s.url", sub->name);
+   if (git_config_set_gently(sb.buf, super_config_url))
+   die(_("failed to register url for submodule path '%s'"),
+ displaypath);
+
+   if (!is_submodule_populated_gently(list_item->name, NULL))
+   goto cleanup;
+
+   prepare_submodule_repo_env(_array);
+   cp.git_cmd = 1;
+   cp.dir = list_item->name;
+   argv_array_pushl(, "submodule--helper",
+"print-default-remote", NULL);
+
+   strbuf_reset();
+   if (capture_command(, , 0))
+   die(_("failed to get the default remote for submodule '%s'"),
+ list_item->name);
+
+   

[GSoC][PATCH 01/13] submodule--helper: introduce get_submodule_displaypath()

[Prathamesh Chavan]

Introduce function get_submodule_displaypath() to replace the code
occurring in submodule_init() for generating displaypath of the
submodule with a call to it.

This new function will also be used in other parts of the system
in later patches.

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 builtin/submodule--helper.c | 33 ++---
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6abdad329..7af4de09b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -220,6 +220,27 @@ static int resolve_relative_url_test(int argc, const char 
**argv, const char *pr
return 0;
 }
 
+static char *get_submodule_displaypath(const char *path, const char *prefix)
+{
+   const char *super_prefix = get_super_prefix();
+
+   if (prefix && super_prefix) {
+   BUG("cannot have prefix '%s' and superprefix '%s'",
+   prefix, super_prefix);
+   } else if (prefix) {
+   struct strbuf sb = STRBUF_INIT;
+   char *displaypath = xstrdup(relative_path(path, prefix, ));
+   strbuf_release();
+   return displaypath;
+   } else if (super_prefix) {
+   int len = strlen(super_prefix);
+   const char *format = is_dir_sep(super_prefix[len - 1]) ? "%s%s" 
: "%s/%s";
+   return xstrfmt(format, super_prefix, path);
+   } else {
+   return xstrdup(path);
+   }
+}
+
 struct module_list {
const struct cache_entry **entries;
int alloc, nr;
@@ -339,16 +360,7 @@ static void init_submodule(const char *path, const char 
*prefix, int quiet)
 
/* Only loads from .gitmodules, no overlay with .git/config */
gitmodules_config();
-
-   if (prefix && get_super_prefix())
-   die("BUG: cannot have prefix and superprefix");
-   else if (prefix)
-   displaypath = xstrdup(relative_path(path, prefix, ));
-   else if (get_super_prefix()) {
-   strbuf_addf(, "%s%s", get_super_prefix(), path);
-   displaypath = strbuf_detach(, NULL);
-   } else
-   displaypath = xstrdup(path);
+   displaypath = get_submodule_displaypath(path, prefix);
 
sub = submodule_from_path(null_sha1, path);
 
@@ -363,7 +375,6 @@ static void init_submodule(const char *path, const char 
*prefix, int quiet)
 * Set active flag for the submodule being initialized
 */
if (!is_submodule_active(the_repository, path)) {
-   strbuf_reset();
strbuf_addf(, "submodule.%s.active", sub->name);
git_config_set_gently(sb.buf, "true");
}
-- 
2.13.0

[GSoC][PATCH 00/13] Update: Week-12

[Prathamesh Chavan]

SUMMARY OF MY PROJECT:

Git submodule subcommands are currently implemented by using shell script
'git-submodule.sh'. There are several reasons why we'll prefer not to
use the shell script. My project intends to convert the subcommands into
C code, thus making them builtins. This will increase Git's portability
and hence the efficiency of working with the git-submodule commands.
Link to the complete proposal: [1]

Mentors:
Stefan Beller 
Christian Couder 

UPDATES:

Following are the updates about my ongoing project:

* Following patches were updated after the previous reviews:
  submodule subcommands:
  - deinit
  - summary
  - foreach

* summary: the function print_submodule_summary() is split-up
  into two separate functions: generate_submodule_summary()
  and print_summary().

* porting of submodule subcommand 'add' is completed and I have
  started with debugging ported function. Currently, the
  entire function cmd_add() is ported to the function
  module_add() in C. Soon, its first patch will be floated
  here as well once debugging is completed. Its progress can be
  viewed at [2].

* displaypath: Last week, there was some confusion produced
  with the way, the value of displaypath is being generated,
  which led to some discussion, which is available at: [3].

PLAN FOR WEEK-13 (8 August 2017 to 14 August 2017):

* patches: IMO, the patches till deinit are reviewed many times,
  and hence will try to get at least these patches merged.

* add: As this subcommand is widely used in the test suite, there
  are many tests this ported function is failing at. Hence,
  debugging the subcommand would be another task for the next week.

* deinit: A bug was identified by Stefan in the last patch-series.
  its details are available at: [4]
  Currenlty, the bug was handled by adding a NEEDSWORK tagged
  comment as suggest. If possible, I will also start working
  on debugging the issue asap.

A complete build report of these series of patches is available at: [5].
Build #151
Branch: week-12

The work is pushed on Github and is available at: [6].

[1]: 
https://docs.google.com/document/d/1krxVLooWl--75Pot3dazhfygR3wCUUWZWzTXtK1L-xU/
[2]: https://github.com/pratham-pc/git/commits/sub-add
[3]: 
https://public-inbox.org/git/CAME+mvXsh53kLJ4se4uKY=SJcvSbHtEZQ6K2CgAPs=1wxux...@mail.gmail.com/
[4]: 
https://public-inbox.org/git/CAGZ79kbyyR54me_+wQDZRrikqKTp_a98yozVfr8P85QHfyyy=q...@mail.gmail.com/
[5]: https://travis-ci.org/pratham-pc/git/builds/
[6]: https://github.com/pratham-pc/git/commits/week-12

Prathamesh Chavan (13):
  submodule--helper: introduce get_submodule_displaypath()
  submodule--helper: introduce for_each_submodule_list()
  submodule: port set_name_rev() from shell to C
  submodule: port submodule subcommand 'status' from shell to C
  submodule: port submodule subcommand 'sync' from shell to C
  submodule: port submodule subcommand 'deinit' from shell to C
  diff: change scope of the function count_lines()
  submodule: port submodule subcommand 'summary' from shell to C
  submodule foreach: correct '$path' in nested submodules from a
subdirectory
  submodule foreach: document '$sm_path' instead of '$path'
  submodule foreach: clarify the '$toplevel' variable documentation
  submodule foreach: document variable '$displaypath'
  submodule: port submodule subcommand 'foreach' from shell to C

 Documentation/git-submodule.txt |   15 +-
 builtin/submodule--helper.c | 1190 ++-
 diff.c  |2 +-
 diff.h  |1 +
 git-submodule.sh|  396 +
 t/t7407-submodule-foreach.sh|   38 +-
 6 files changed, 1222 insertions(+), 420 deletions(-)

-- 
2.13.0

[GSoC][PATCH 02/13] submodule--helper: introduce for_each_submodule_list()

[Prathamesh Chavan]

Introduce function for_each_submodule_list() and
replace a loop in module_init() with a call to it.

The new function will also be used in other parts of the
system in later patches.

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 builtin/submodule--helper.c | 39 +--
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 7af4de09b..e41572f7a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -14,6 +14,9 @@
 #include "refs.h"
 #include "connect.h"
 
+typedef void (*submodule_list_func_t)(const struct cache_entry *list_item,
+ void *cb_data);
+
 static char *get_default_remote(void)
 {
char *dest = NULL, *ret;
@@ -352,17 +355,30 @@ static int module_list(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
-static void init_submodule(const char *path, const char *prefix, int quiet)
+static void for_each_submodule_list(const struct module_list list,
+   submodule_list_func_t fn, void *cb_data)
 {
+   int i;
+   for (i = 0; i < list.nr; i++)
+   fn(list.entries[i], cb_data);
+}
+
+struct init_cb {
+   const char *prefix;
+   unsigned int quiet: 1;
+};
+#define INIT_CB_INIT { NULL, 0 }
+
+static void init_submodule(const struct cache_entry *list_item, void *cb_data)
+{
+   struct init_cb *info = cb_data;
const struct submodule *sub;
struct strbuf sb = STRBUF_INIT;
char *upd = NULL, *url = NULL, *displaypath;
 
-   /* Only loads from .gitmodules, no overlay with .git/config */
-   gitmodules_config();
-   displaypath = get_submodule_displaypath(path, prefix);
+   displaypath = get_submodule_displaypath(list_item->name, info->prefix);
 
-   sub = submodule_from_path(null_sha1, path);
+   sub = submodule_from_path(null_sha1, list_item->name);
 
if (!sub)
die(_("No url found for submodule path '%s' in .gitmodules"),
@@ -374,7 +390,7 @@ static void init_submodule(const char *path, const char 
*prefix, int quiet)
 *
 * Set active flag for the submodule being initialized
 */
-   if (!is_submodule_active(the_repository, path)) {
+   if (!is_submodule_active(the_repository, list_item->name)) {
strbuf_addf(, "submodule.%s.active", sub->name);
git_config_set_gently(sb.buf, "true");
}
@@ -416,7 +432,7 @@ static void init_submodule(const char *path, const char 
*prefix, int quiet)
if (git_config_set_gently(sb.buf, url))
die(_("Failed to register url for submodule path '%s'"),
displaypath);
-   if (!quiet)
+   if (!info->quiet)
fprintf(stderr,
_("Submodule '%s' (%s) registered for path 
'%s'\n"),
sub->name, url, displaypath);
@@ -445,10 +461,10 @@ static void init_submodule(const char *path, const char 
*prefix, int quiet)
 
 static int module_init(int argc, const char **argv, const char *prefix)
 {
+   struct init_cb info = INIT_CB_INIT;
struct pathspec pathspec;
struct module_list list = MODULE_LIST_INIT;
int quiet = 0;
-   int i;
 
struct option module_init_options[] = {
OPT__QUIET(, N_("Suppress output for initializing a 
submodule")),
@@ -473,8 +489,11 @@ static int module_init(int argc, const char **argv, const 
char *prefix)
if (!argc && git_config_get_value_multi("submodule.active"))
module_list_active();
 
-   for (i = 0; i < list.nr; i++)
-   init_submodule(list.entries[i]->name, prefix, quiet);
+   info.prefix = prefix;
+   info.quiet = !!quiet;
+
+   gitmodules_config();
+   for_each_submodule_list(list, init_submodule, );
 
return 0;
 }
-- 
2.13.0

[GSoC][PATCH 03/13] submodule: port set_name_rev() from shell to C

[Prathamesh Chavan]

Function set_name_rev() is ported from git-submodule to the
submodule--helper builtin. The function get_name_rev() generates the
value of the revision name as required, and the function
print_name_rev() handles the formating and printing of the obtained
revision name.

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 builtin/submodule--helper.c | 63 +
 git-submodule.sh| 16 ++--
 2 files changed, 65 insertions(+), 14 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index e41572f7a..421eee1e2 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -244,6 +244,68 @@ static char *get_submodule_displaypath(const char *path, 
const char *prefix)
}
 }
 
+static char *get_name_rev(const char *sub_path, const char* object_id)
+{
+   struct strbuf sb = STRBUF_INIT;
+   const char ***d;
+
+   static const char *describe_bare[] = {
+   NULL
+   };
+
+   static const char *describe_tags[] = {
+   "--tags", NULL
+   };
+
+   static const char *describe_contains[] = {
+   "--contains", NULL
+   };
+
+   static const char *describe_all_always[] = {
+   "--all", "--always", NULL
+   };
+
+   static const char **describe_argv[] = {
+   describe_bare, describe_tags, describe_contains,
+   describe_all_always, NULL
+   };
+
+   for (d = describe_argv; *d; d++) {
+   struct child_process cp = CHILD_PROCESS_INIT;
+   prepare_submodule_repo_env(_array);
+   cp.dir = sub_path;
+   cp.git_cmd = 1;
+   cp.no_stderr = 1;
+
+   argv_array_push(, "describe");
+   argv_array_pushv(, *d);
+   argv_array_push(, object_id);
+
+   if (!capture_command(, , 0) && sb.len) {
+   strbuf_strip_suffix(, "\n");
+   return strbuf_detach(, NULL);
+   }
+   }
+
+   strbuf_release();
+   return NULL;
+}
+
+static int print_name_rev(int argc, const char **argv, const char *prefix)
+{
+   char *namerev;
+   if (argc != 3)
+   die("print-name-rev only accepts two arguments:  ");
+
+   namerev = get_name_rev(argv[1], argv[2]);
+   if (namerev && namerev[0])
+   printf(" (%s)", namerev);
+   printf("\n");
+
+   free(namerev);
+   return 0;
+}
+
 struct module_list {
const struct cache_entry **entries;
int alloc, nr;
@@ -1242,6 +1304,7 @@ static struct cmd_struct commands[] = {
{"relative-path", resolve_relative_path, 0},
{"resolve-relative-url", resolve_relative_url, 0},
{"resolve-relative-url-test", resolve_relative_url_test, 0},
+   {"print-name-rev", print_name_rev, 0},
{"init", module_init, SUPPORT_SUPER_PREFIX},
{"remote-branch", resolve_remote_submodule_branch, 0},
{"push-check", push_check, 0},
diff --git a/git-submodule.sh b/git-submodule.sh
index e131760ee..e988167e0 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -759,18 +759,6 @@ cmd_update()
}
 }
 
-set_name_rev () {
-   revname=$( (
-   sanitize_submodule_env
-   cd "$1" && {
-   git describe "$2" 2>/dev/null ||
-   git describe --tags "$2" 2>/dev/null ||
-   git describe --contains "$2" 2>/dev/null ||
-   git describe --all --always "$2"
-   }
-   ) )
-   test -z "$revname" || revname=" ($revname)"
-}
 #
 # Show commit summary for submodules in index or working tree
 #
@@ -1042,14 +1030,14 @@ cmd_status()
fi
if git diff-files --ignore-submodules=dirty --quiet -- 
"$sm_path"
then
-   set_name_rev "$sm_path" "$sha1"
+   revname=$(git submodule--helper print-name-rev 
"$sm_path" "$sha1")
say " $sha1 $displaypath$revname"
else
if test -z "$cached"
then
sha1=$(sanitize_submodule_env; cd "$sm_path" && 
git rev-parse --verify HEAD)
fi
-   set_name_rev "$sm_path" "$sha1"
+   revname=$(git submodule--helper print-name-rev 
"$sm_path" "$sha1")
say "+$sha1 $displaypath$revname"
fi
 
-- 
2.13.0

Re: [PATCH] test-path-utils: handle const parameter of basename and dirname

[Johannes Schindelin]

Hi René,

On Mon, 7 Aug 2017, René Scharfe wrote:

> The parameter to basename(3) and dirname(3) traditionally had the type
> "char *", but on OpenBSD it's been "const char *" for years.  That
> causes (at least) Clang to throw an incompatible-pointer-types warning
> for test-path-utils, where we try to pass around pointers to these
> functions.
> 
> Avoid this warning (which is fatal in DEVELOPER mode) by ignoring the
> promise of OpenBSD's implementations to keep input strings unmodified
> and enclosing them in POSIX-compatible wrappers.
> 
> Signed-off-by: Rene Scharfe 
> ---

This patch is Fine By Me.

Thanks,
Dscho

Re: [PATCH 0/6] clean up parsing of maybe_bool

[Junio C Hamano]

Stefan Beller  writes:

> The series looks fine to me overall, though patch 5 is overly gentle IMHO.
> We could have removed it right there as Junio is very good at resolving
> conflicts or producing dirty merges for such a situation.
> But delaying it until no other series' are in flight is fine with me, too.

If you remove the old one, it would cause compilation error due to
removal of the declaration of the old one when other series that are
in flight adds new callsites to it.  Which makes life a bit easier
for the integrators when it is trivial to convert these callsites to
use the new one.  If the way the old one and the new one are called
are vastly different, of course, leaving the compatibility layer
that no longer is used after the series will make it easier to live
with other topics in flight, on the other hand.

I am fine with either in this case, but I probably would have opted
for removal at the end of this series if I were doing this series,
because

-   git_config_maybe_bool(K,V)
+   git_parse_maybe_bool(V)

that may have to happen during evil merges would have been trivial.

Thanks.

Re: [PATCH] Fix delta integer overflows

[Johannes Schindelin]

Hi Junio,

On Mon, 7 Aug 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> >> The patch obviously makes the code better and self consistent in
> >> that "struct delta_index" has src_size as ulong, and this function
> >> takes trg_size as ulong, and it was plain wrong for the code to
> >> assume that "i", which is uint, can receive it safely.
> >> 
> >> In the longer term we might want to move to size_t or even
> >> uintmax_t, as the ulong on a platform may not be long enough in
> >> order to express the largest file size the platform can have, but
> >> this patch (1) is good even without such a change, and (2) gives a
> >> good foundation to build on if we want such a change on top.
> >> 
> >> Thanks.  Will queue.
> >
> > This is sad. There is no "may not be long enough". We already know a
> > platform where unsigned long is not long enough, don't we? Why leave this
> > patch in this intermediate state?
> 
> This is a good foundation to build on, and I never said no further
> update on top of this patch is desired.  Look for "(2)" in what you
> quoted.

So are you saying that starting with v2.14.0, you accept patches into `pu`
for which you would previously have required multiple iterations before
even considering it for `pu`?

Frankly, I am a bit surprised that this obvious change from `unsigned
long` to `size_t` is not required in this case before queuing, but if the
rules have changed to lower the bar for patch submissions, I am all for
it. I always felt that we are wasting contributors' time a little too
freely and too deliberately.

Ciao,
Dscho

Re: [PATCH] Drop some dashes from built-in invocations in scripts

[Johannes Schindelin]

Hi Junio,

I feel a bit talked to my hand, as the only reply I was graced was a "I
think I already did". So this will be my last reply on this matter for a
while.

On Mon, 7 Aug 2017, Junio C Hamano wrote:

> IIUC, you will need "$GIT_EXEC_PATH/git-checkout" on the filesystem if
> you want your "git co" alias to work, as we spawn built-in as a dashed
> external.

And of course this is just the status quo, not an argument why it should
be so for eternity. Because that would be circular reasoning and prevent
us from improving things.

It is still arguably wrong to call the dashed form for builtins when we
already have enough information at our hands to tell that it is a builtin:

https://github.com/git-for-windows/git/commit/bad2c6978ec

Granted, this duplicates code a little, as it was developed under time
pressure (and it is necessary to allow the test suite to be run on Git
built in Visual Studio using an installed Git for Windows for the Unix
utilities). As above, though, the current state of this patch does not
prevent any improvement in the future.

Ciao,
Dscho

Re: Can the '--set-upstream' option of branch be removed ?

[Junio C Hamano]

Kaartic Sivaraam  writes:

> I refactored builtin/branch.c to remove the '--set-upstream'
> option,successfully. The corresponding patch follows. 
>
> There's just one issue with the version of git that doesn't
> have the '--set-upstream' option. It's described in the commit
> log message of the following patch.

which is...

> Note that, 'git branch' still *accepts* '--set-upstream' as a consequence
> of "unique prefix can be abbrievated in option names". '--set-upstream'
> is a unique prefix of '--set-upstream-to' after '--set-upstream' has
> been removed.

... this.

Thanks for spotting the issue.  

I think in the longer term we still want to remove --set-upstream as
many people seem to say that its behaviour has been uttering
confusing to them and that is why we keep giving the warning any
time it is used.

> I guess it would be difficult to detect the removal of the option in
> case it's used in scripts and might cause confusion to users?

If we want to follow through the transition, because of the issue
you spotted, we'd need one extra step to make sure users won't be
hurt before removal: we would need to still recognize --set-upstream
as an option distinct from --set-upstream-to, and actively fail the
request, telling them that the former option no longer is supported.

Then after waiting for a few years, we may be able to re-introduce
the "--set-upstream" option that takes the arguments in the same
order as "--set-upstream-to", which would be the ideal endgame
(assuming that the reason why we started deprecating "--set-upstream"
and encouraged users to use "--set-upstream-to" still holds).

Re: [RFC] imap-send: escape backslash in password

[Jeff King]

On Sun, Aug 06, 2017 at 09:12:16PM +0200, Nicolas Morey-Chaisemartin wrote:

> Also it probably make sense to have at least one release where --curl
> is the default. Until your mail I had no idea this option existed so I
> never tried it out.
> Making it the default will make sure almost everyone is using it and
> that there is feature-parity.

Yeah, I had thought that the curl implementation _was_ the default if
you have curl. But we just build it by default, and you have to manually
enable it. So I agree it has not gotten nearly as much testing as I had
thought, and as you found, it diverges from the earlier implementation
in quite a few areas.

So I think we would need to take any deprecation much more slowly than I
had first thought (and your patches in the nearby thread are moving in a
good direction).

-Peff

Re: [PATCH 2/6] t5334: document that git push --signed=1 does not work

[Junio C Hamano]

Martin Ågren  writes:

> When accepting booleans as command-line or config options throughout
> Git, there are several documented synonyms for true and false.
> However, one particular user is slightly broken: `git push --signed=..`
> does not understand the integer synonyms for true and false.
>
> This is hardly wanted. The --signed option has a different notion of
> boolean than all other arguments and config options, including the
> config option corresponding to it, push.gpgSign.
>
> Add a test documenting the failure to handle --signed=1.

My knee-jerk reaction is that parse_maybe_bool() is broken in that
it should do everything that config_maybe_bool() does.  I wonder why
we do not call git_parse_int() like git_config_maybe_bool() does?

... Ahh, that is because bool_or_int() wants to know that "1" is an
int and not a bool, and parse_maybe_bool() is designed to cater to
the need of that thing, in addition to config_maybe_bool().  And the
"--signed=" thing wants parse_bool_or_string(), not bool_or_int(),
so we should treat "1" as true and "0" as false.  There is no way to
do so in the current code and that is why you have the new _text()
thing in patches 3-4/6.

Makes sense.


> diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
> index 464ffdd14..5dce55e1d 100755
> --- a/t/t5534-push-signed.sh
> +++ b/t/t5534-push-signed.sh
> @@ -71,6 +71,13 @@ test_expect_success 'push --signed fails with a receiver 
> without push certificat
>   test_i18ngrep "the receiving end does not support" err
>  '
>  
> +test_expect_failure 'push --signed=1 is accepted' '
> + prepare_dst &&
n> +mkdir -p dst/.git/hooks &&
> + test_must_fail git push --signed=1 dst noop ff +noff 2>err &&
> + test_i18ngrep "the receiving end does not support" err
> +'
> +
>  test_expect_success GPG 'no certificate for a signed push with no update' '
>   prepare_dst &&
>   mkdir -p dst/.git/hooks &&

Re: [PATCH 1/4] imap-send: add wrapper to get server credentials if needed

[Jeff King]

On Mon, Aug 07, 2017 at 09:55:56PM +0200, Nicolas Morey-Chaisemartin wrote:

> > On the other hand, if we're hoping to get rid of this code in favor of
> > the curl-based approach, then it's not worth spending time on
> > cosmetic refactoring, as long as it still behaves correctly in the
> > interim.
> 
> Looking at the code, it seems the tunnel mode always uses the legacy imap 
> approach.
> This would have to be ported to curl and stabilized before dropping the 
> legacy code.

Urgh. That's an important mode, I'd think, and one that I have a feeling
curl may not be interested in supporting, just because of it's
complexity. And even if they did, it would take a while for that curl
version to become available.

So maybe the idea of deprecating the non-curl implementation is not
something that can happen anytime soon. :(

> In the meantime, it might be worth doing a bit of cleanup.

In which case, yeah, it is definitely worth cleaning up the existing
code. But I also agree with you that it's worth making sure the curl
version behaves as similarly as possible.

-Peff

Re: [PATCH 4/4] imap-send: use curl by default

[Jeff King]

On Mon, Aug 07, 2017 at 04:04:05PM +0200, Nicolas Morey-Chaisemartin wrote:

> Signed-off-by: Nicolas Morey-Chaisemartin 

Thanks for moving forward with this.

Can you please flesh out your commit messages with some of the reasoning
and related discussion? I know from a nearby thread why we want to flip
the default, but people reading `git log` much later will not have that
context.

> diff --git a/imap-send.c b/imap-send.c
> index 90b8683ed..4ebc16437 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -35,13 +35,7 @@ typedef void *SSL;
>  #include "http.h"
>  #endif
>  
> -#if defined(USE_CURL_FOR_IMAP_SEND) && defined(NO_OPENSSL)
> -/* only available option */
>  #define USE_CURL_DEFAULT 1
> -#else
> -/* strictly opt in */
> -#define USE_CURL_DEFAULT 0
> -#endif

I agree with the comments Martin made here. I think there are really two
levels of "default" we need to care about:

  1. Build-time: do we default to requiring curl if you want imap-send
 at all?

  2. Run-time: if build with both implementations, which do we use by
 default? Related, if there is only one implementation, what should
 the default be?

I think the answer to (1) is that we still want to build imap-send
without USE_CURL_FOR_IMAP_SEND if the user doesn't have curl installed.
And your patch leaves that, which is good. Though if we are deprecating
it, we may want to issue a deprecation warning (eventually; we can still
switch the run-time default now, get more data on whether a
deprecation/switch is a good idea, and then later decide to deprecate).

For (2), you're trying to switch the default when both are built. But I
think it's important to continue to default to the old-style
implementation if that's the only thing that was built. Otherwise it
effectively becomes the build-time deprecation warning, and we're not
quite ready for that.

So I think this maybe needs to be:

  #if defined(USE_CURL_FOR_IMAP_SEND)
  /* Always default to curl if it's available. */
  #define USE_CURL_DEFAULT 1
  #else
  /* We don't have curl, so continue to use the historical implementation */
  #define USE_CURL_DEFAULT 0
  #endif

-Peff

Re: [PATCH 1/4] imap-send: add wrapper to get server credentials if needed

[Nicolas Morey-Chaisemartin]

Le 07/08/2017 à 21:42, Jeff King a écrit :
> On Mon, Aug 07, 2017 at 07:18:32PM +0200, Martin Ågren wrote:
>
 "cred.username" is checked further down, but now it will always be NULL,
 no?
>>> You're right I missed this.
>>> Not sure if this is needed though.
>>> From what I understand this means the username/password are store for the 
>>> next access to credential. but in the current state, there is only one.
>>> Maybe the credential_approved can be dropped ?
>> I'm no credentials-expert, but api-credentials.txt says this:
>>
>> "Credential helpers are programs executed by Git to fetch or save
>> credentials from and to long-term storage (where "long-term" is simply
>> longer than a single Git process; e.g., credentials may be stored
>> in-memory for a few minutes, or indefinitely on disk)."
>>
>> So the calls to approve/reject probably do matter in some scenarios.
> Right, this is important. credential_fill() may actually prompt the
> user, and we'd want to then pass along that credential for storage. Or
> the user may have changed their password, and the credential_reject() is
> the only thing that prevents us from trying an out-dated password over
> and over.
>
>> The current code is a bit non-obvious as we just discovered since it
>> duplicates the strings (for good reasons, I believe) and then still
>> refers to the originals (also for good reasons, I believe). I suppose
>> your new function could be called like
>>
>> server_fill_credential(, srvc);
>>
>> That should limit the impact of the change, but I'm not sure it's a
>> brilliant interface. Just my 2c.
> That would work. I'm also tempted to say that imap_server_conf could
> just store the "struct credential" inside it. We could even potentially
> drop its parallel user/pass fields to minimize confusion.
>
> Once upon a time imap-send was a fork of another program, which is why
> most of its code isn't well-integrated with Git. But I think at this
> point there's very little chance of merging changes back and forth
> between the two.
>
> On the other hand, if we're hoping to get rid of this code in favor of
> the curl-based approach, then it's not worth spending time on
> cosmetic refactoring, as long as it still behaves correctly in the
> interim.
>
> -Peff

Looking at the code, it seems the tunnel mode always uses the legacy imap 
approach.
This would have to be ported to curl and stabilized before dropping the legacy 
code.
In the meantime, it might be worth doing a bit of cleanup.
And as I said in patch 4, I have a current IMAP account where it works without 
curl but not with curl (for unknown reason yet).
Meaning legacy needs to stay for a while. But switching to curl as default to 
get out all the bugs (hence this slightly broken patch series)

I think it would make sense to get patch 1 (fixed), 2 and 4 in to really test 
out the curl implementation and take some time to prepare another serie with 
code cleanups: dropping imap_server_conf parameters, storing cred therem etc.)

Nicolas

[PATCH] t3700: fix broken test under !POSIXPERM

[René Scharfe]

76e368c378 (t3700: fix broken test under !SANITY) explains that the test
'git add --chmod=[+-]x changes index with already added file' can fail
if xfoo3 is still present as a symlink from a previous test and deletes
it with rm(1).  That still leaves it present in the index, which causes
the test to fail if POSIXPERM is not defined.  Get rid of it by calling
"git reset --hard" instead, as 76e368c378 already mentioned in passing.

Signed-off-by: Rene Scharfe 
---
 t/t3700-add.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index f3a4b4a913..4111fa3b7a 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -355,7 +355,7 @@ test_expect_success POSIXPERM,SYMLINKS 'git add --chmod=+x 
with symlinks' '
 '
 
 test_expect_success 'git add --chmod=[+-]x changes index with already added 
file' '
-   rm -f foo3 xfoo3 &&
+   git reset --hard &&
echo foo >foo3 &&
git add foo3 &&
git add --chmod=+x foo3 &&
-- 
2.14.0

Re: [PATCH 3/4] imap_send: setup_curl: use server_conf parameter instead of the global variable

[Jeff King]

On Mon, Aug 07, 2017 at 07:06:07PM +0200, Nicolas Morey-Chaisemartin wrote:

> >> -   server_fill_credential();
> >> -   curl_easy_setopt(curl, CURLOPT_USERNAME, server.user);
> >> -   curl_easy_setopt(curl, CURLOPT_PASSWORD, server.pass);
> >> +   server_fill_credential(srvc);
> >> +   curl_easy_setopt(curl, CURLOPT_USERNAME, srvc->user);
> >> +   curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);
> > Here you change the server_fill_credential-call that you just added.
> > Maybe do this patch earlier, perhaps even as patch 1?
> >
> > I'm snipping lots of s/server/srvc/-changes... There's a less noisy
> > way of addressing the fact that srvc is unused: dropping it. I'm not
> > saying that's a good idea, but it could be considered, then explained
> > why this approach is better. There are some other functions which
> > access "server" directly, and some which take (and use!) a "srvc".
> > Maybe make the whole file consistent?
> >
> That's why I applied it after #2. I was not sure if this one made
> sense or not. And it  can be dropped with the rest of the series still
> applying.
> I don't know what is the right approach here. Someone with more
> knowledge of why there is a mix of global variable and local can maybe
> help ?

I suspect it's just code in need of a cleanup. But let's cc the original
author of 1e16b255b (git-imap-send: use libcurl for implementation,
2014-11-09) to see if he has any comments[1].

-Peff

[1] Bernhard, the whole series is at:

  
https://public-inbox.org/git/38d3ae5b-4020-63cc-edfa-0a77e4279...@morey-chaisemartin.com/

The general idea is to make sure the original and curl imap-send
implementations have feature parity, make the curl version the
default, and then hopefully eventually drop the non-curl one
entirely.

Re: [PATCH] Drop some dashes from built-in invocations in scripts

[Junio C Hamano]

Johannes Schindelin  writes:

> So I would love to hear the arguments for keeping the dashed forms of
> builtins, even if the only surviving argument may be "I dig in my feet
> because I always said we'd keep them".

I think I already did ;-)

Re: [PATCH] Fix delta integer overflows

[Junio C Hamano]

Johannes Schindelin  writes:

>> The patch obviously makes the code better and self consistent in
>> that "struct delta_index" has src_size as ulong, and this function
>> takes trg_size as ulong, and it was plain wrong for the code to
>> assume that "i", which is uint, can receive it safely.
>> 
>> In the longer term we might want to move to size_t or even
>> uintmax_t, as the ulong on a platform may not be long enough in
>> order to express the largest file size the platform can have, but
>> this patch (1) is good even without such a change, and (2) gives a
>> good foundation to build on if we want such a change on top.
>> 
>> Thanks.  Will queue.
>
> This is sad. There is no "may not be long enough". We already know a
> platform where unsigned long is not long enough, don't we? Why leave this
> patch in this intermediate state?

This is a good foundation to build on, and I never said no further
update on top of this patch is desired.  Look for "(2)" in what you
quoted.

Re: [PATCH 1/4] imap-send: add wrapper to get server credentials if needed

[Jeff King]

On Mon, Aug 07, 2017 at 07:18:32PM +0200, Martin Ågren wrote:

> >> "cred.username" is checked further down, but now it will always be NULL,
> >> no?
> >
> > You're right I missed this.
> > Not sure if this is needed though.
> > From what I understand this means the username/password are store for the 
> > next access to credential. but in the current state, there is only one.
> > Maybe the credential_approved can be dropped ?
> 
> I'm no credentials-expert, but api-credentials.txt says this:
> 
> "Credential helpers are programs executed by Git to fetch or save
> credentials from and to long-term storage (where "long-term" is simply
> longer than a single Git process; e.g., credentials may be stored
> in-memory for a few minutes, or indefinitely on disk)."
> 
> So the calls to approve/reject probably do matter in some scenarios.

Right, this is important. credential_fill() may actually prompt the
user, and we'd want to then pass along that credential for storage. Or
the user may have changed their password, and the credential_reject() is
the only thing that prevents us from trying an out-dated password over
and over.

> The current code is a bit non-obvious as we just discovered since it
> duplicates the strings (for good reasons, I believe) and then still
> refers to the originals (also for good reasons, I believe). I suppose
> your new function could be called like
> 
> server_fill_credential(, srvc);
> 
> That should limit the impact of the change, but I'm not sure it's a
> brilliant interface. Just my 2c.

That would work. I'm also tempted to say that imap_server_conf could
just store the "struct credential" inside it. We could even potentially
drop its parallel user/pass fields to minimize confusion.

Once upon a time imap-send was a fork of another program, which is why
most of its code isn't well-integrated with Git. But I think at this
point there's very little chance of merging changes back and forth
between the two.

On the other hand, if we're hoping to get rid of this code in favor of
the curl-based approach, then it's not worth spending time on
cosmetic refactoring, as long as it still behaves correctly in the
interim.

-Peff

Re: Partial clone design (with connectivity check for locally-created objects)

[Junio C Hamano]

Ben Peart  writes:

> My concern with this proposal is the combination of 1) writing a new
> pack file for every git command that ends up bringing down a missing
> object and 2) gc not compressing those pack files into a single pack
> file.

Your noticing these is a sign that you read the outline of the
design correctly, I think.  

The basic idea is that the local fsck should tolerate missing
objects when they are known to be obtainable from that external
service, but should still be able to diagnose missing objects that
we do not know if the external service has, especially the ones that
have been newly created locally and not yet made available to them
by pushing them back.

So we need a way to tell if an object that we do not have (but we
know about) can later be obtained from the external service.
Maintaining an explicit list of such objects obviously is one way,
but we can get the moral equivalent by using pack files.  After
receiving a pack file that has a commit from such an external
service, if the commit refers to its parent commit that we do not
have locally, the design proposes us to consider that the parent
commit that is missing is available at the external service that
gave the pack to us.  Similarly for missing trees, blobs, and any
objects that are supposed to be "reachable" from objects in such a
packfile.  

We can extend the approach to cover loose objects if we wanted to;
just define an alternate object store used internally for this
purpose and drop loose objects obtained from such an external
service in that object store.

Because we do not want to leave too many loose objects and small
packfiles lying around, we will need a new way of packing these.
Just enumerate these objects known to have come from the external
service (by being in packfiles marked as such or being loose objects
in the dedicated alternate object store), and create a single larger
packfile, which is marked as "holding the objects that are known to
be in the external service".  We do not have such a mode of gc, and
that is a new development that needs to happen, but we know that is
doable.

> That thinking did lead me back to wondering again if we could live
> with a repo specific flag.  If any clone/fetch was "partial" the flag
> is set and fsck ignore missing objects whether they came from a
> "partial" remote or not.

The only reason people run "git fsck" is to make sure that their
local repository is sound and they can rely on the objects you have
as the base of building new stuff on top of.  That is why we are
trying to find a way to make sure "fsck" can be used to detect
broken or missing objects that cannot be obtained from the
lazy-object store, without incurring undue overhead for normal
codepath (i.e. outside fsck).

It is OK to go back to wondering again, but I think that essentially
tosses "git fsck" out of the window and declares that it is OK to
hope that local objects will never go bad.  We can make such an
declaration anytime, but I do not want to see us doing so without
first trying to solve the issue without punting.

Re: [PATCH] Fix delta integer overflows

[Johannes Schindelin]

Hi,

On Mon, 7 Aug 2017, Junio C Hamano wrote:

> Martin Koegler  writes:
> 
> > From: Martin Koegler 
> >
> > The current delta code produces incorrect pack objects for files > 4GB.
> >
> > Signed-off-by: Martin Koegler 
> > ---
> >  diff-delta.c | 23 ---
> >  1 file changed, 12 insertions(+), 11 deletions(-)
> >
> > Just pass any file > 4 GB to the delta-compression [by increasing the delta 
> > limits].
> > As file size, a truncated 32bit value will be encoded, leading to broken 
> > pack files.
> 
> The patch obviously makes the code better and self consistent in
> that "struct delta_index" has src_size as ulong, and this function
> takes trg_size as ulong, and it was plain wrong for the code to
> assume that "i", which is uint, can receive it safely.
> 
> In the longer term we might want to move to size_t or even
> uintmax_t, as the ulong on a platform may not be long enough in
> order to express the largest file size the platform can have, but
> this patch (1) is good even without such a change, and (2) gives a
> good foundation to build on if we want such a change on top.
> 
> Thanks.  Will queue.

This is sad. There is no "may not be long enough". We already know a
platform where unsigned long is not long enough, don't we? Why leave this
patch in this intermediate state?

If you want to work on data in memory, then size_t is the appropriate data
type. We already use it elsewhere. Let's use it here, too, without the
intermediate bump from the incorrect `int` to the equally incorrect
`long`.

Ciao,
Johannes

Re: [git-for-windows] [ANNOUNCE] Git for Windows 2.14.0

[Johannes Schindelin]

Hi Hannes,

On Mon, 7 Aug 2017, Johannes Sixt wrote:

> Am 07.08.2017 um 12:02 schrieb Johannes Schindelin:
> > On Sun, 6 Aug 2017, Johannes Sixt wrote:
> > > Am 06.08.2017 um 01:00 schrieb Johannes Schindelin:
> > > > * Comes with [BusyBox
> > > > v1.28.0pre.15857.9480dca7c](https://github.com/
> > > >   git-for-windows/busybox-w32/commit/9480dca7c].
> > >
> > > What is the implication of this addition? I guess it is not just for the
> > > fun of it. Does it mean that all POSIX command line tools invoked by Git
> > > including a POSIX shell are now routed through busybox instead of the
> > > MSYS2 variant?
> > 
> > As I wrote a little later:
> > 
> > * Git for Windows releases now also include an experimental [BusyBox-based
> >
> > MinGit](https://github.com/git-for-windows/git/wiki/MinGit#experimental-busybox-based-mingit).
> 
> Thanks for the clue. It's an interesting concept. I would be interested in
> replacing my old MSYS environment by BusyBox. At best, it would be just a
> matter of replacing sh.exe.

OpenSSH and GPG are also required for Git for Windows to function well.
You may not need them, but others do. Also, you may be content with
running the shell in your Windows Console, but most users use MinTTY (and
would have used rxvt if we ever had gotten that to work).

Given your circumstances, I would estimate that you could already use
a BusyBox-based system. You have been traditionally very comfortable with
running your own setup, and putting it together yourself, so you could
cherry-pick the bits and pieces.

The only exception may be `vi`. While BusyBox comes with a `vi` applet, I
disabled it because it does not work in MinTTY, and it also offers a far
cry from the vim.exe functionality I am used to. So you may want to revert
https://github.com/git-for-windows/busybox-w32/commit/4dccf1500f4 and
rebuild BusyBox-w32 yourself.

Ciao,
Dscho

Re: [PATCH] Drop some dashes from built-in invocations in scripts

[Johannes Schindelin]

Hi Junio,

On Mon, 7 Aug 2017, Junio C Hamano wrote:

> Michael Forney  writes:
> 
> > This way, they still work even if the built-in symlinks aren't
> > installed.
> >
> > Signed-off-by: Michael Forney 
> > ---
> > It looks like there was an effort to do this a number of years ago (through
> > `make remove-dashes`). These are just a few I noticed were still left in the
> > .sh scripts.
> 
> Our goal was *not* to have *no* "git-foo" on the filesystem,
> though. It happened in v1.6.0 timeframe and it was about removing
> "git-foo" from end-user's $PATH.

That was in the v1.6.0 timeframe. It is past time to reconsider the goal,
though, as there is really very little use in keeping the dashed forms.

And it does hurt in some circumstances. Take for example .zip files: they
do not support hard-links. So if you need to distribute Git binaries in
.zip files, you are not only affected negatively by the less-than-stellar
compression ratio compared to .bz2 let alone LZMA, Git adds insult to
injury by *forcing* an additional inflation by pointlessly adding the
builtins *again*.

> Earlier there was a more ambitious proposal to remove all "git-foo"
> even from $GIT_EXEC_PATH for built-in commands, but that plan was
> scuttled [*1*].

That *1* is not a good reference, I am afraid. It says very little in
addition to paraphrased commands to stop responding (when a more civilized
call back to rational arguments might have been a lot more productive).

In that light, would you kindly explain in your own words what is your
current thinking on shipping dashed forms of builtins?

I mean, I can understand for git-upload-pack, to help with determining
permissions on server sides (it is easier to filter out all `git` commands
than to painstakingly look whether argv[1] equals `upload-pack`). It's
sort of a very unfortunate outlier.

But I cannot understand at all why we insist on installing hardlinked
copies (or not so hardlinked copies when hardlinks are unavailable) for
builtins, when these copies really outlived their usefulness a long, long
time ago.

So I would love to hear the arguments for keeping the dashed forms of
builtins, even if the only surviving argument may be "I dig in my feet
because I always said we'd keep them".

Ciao,
Dscho

Re: Partial clone design (with connectivity check for locally-created objects)

[Jonathan Nieder]

Hi,

Ben Peart wrote:
>> On Fri, 04 Aug 2017 15:51:08 -0700
>> Junio C Hamano  wrote:
>>> Jonathan Tan  writes:

 "Imported" objects must be in a packfile that has a ".remote"
 file with arbitrary text (similar to the ".keep" file). They come from
 clones, fetches, and the object loader (see below).
 ...

 A "homegrown" object is valid if each object it references:
  1. is a "homegrown" object,
  2. is an "imported" object, or
  3. is referenced by an "imported" object.
>>>
>>> Overall it captures what was discussed, and I think it is a good
>>> start.
>
> I missed the offline discussion and so am trying to piece together
> what this latest design is trying to do.  Please let me know if I'm
> not understanding something correctly.

I believe
https://public-inbox.org/git/cover.1501532294.git.jonathanta...@google.com/
and the surrounding thread (especially
https://public-inbox.org/git/xmqqefsudjqk@gitster.mtv.corp.google.com/)
is the discussion Junio is referring to.

[...]
> This segmentation is what is driving the need for the object loader
> to build a new local pack file for every command that has to fetch a
> missing object.  For example, we can't just write a tree object from
> a "partial" clone into the loose object store as we have no way for
> fsck to treat them differently and ignore any missing objects
> referenced by that tree object.

That's related and how it got lumped into this proposal, but it's not
the only motivation.

Other aspects:

 1. using pack files instead of loose objects means we can use deltas.
This is the primary motivation.

 2. pack files can use reachability bitmaps (I realize there are
obstacles to getting benefit out of this because git's bitmap
format currently requires a pack to be self-contained, but I
thought it was worth mentioning for completeness).

 3. existing git servers are oriented around pack files; they can
more cheaply serve objects from pack files in pack format,
including reusing deltas from them.

 4. file systems cope better with a few large files than many small
files

[...]
> We all know that git doesn't scale well with a lot of pack files as
> it has to do a linear search through all the pack files when
> attempting to find an object.  I can see that very quickly, there
> would be a lot of pack files generated and with gc ignoring
> "partial" pack files, this would never get corrected.

Yes, that's an important point.  Regardless of this proposal, we need
to get more aggressive about concatenating pack files (e.g. by
implementing exponential rollup in "git gc --auto").

> In our usage scenarios, _all_ of the objects come from "partial"
> clones so all of our objects would end up in a series of "partial"
> pack files and would have pretty poor performance as a result.

Can you say more about this?  Why would the pack files (or loose
objects, for that matter) never end up being consolidated into few
pack files?

[...]
> That thinking did lead me back to wondering again if we could live
> with a repo specific flag.  If any clone/fetch was "partial" the
> flag is set and fsck ignore missing objects whether they came from a
> "partial" remote or not.
>
> I'll admit it isn't as robust if someone is mixing and matching
> remotes from different servers some of which are partial and some of
> which are not.  I'm not sure how often that would actually happen
> but I _am_ certain a single repo specific flag is a _much_ simpler
> model than anything else we've come up with so far.

The primary motivation in this thread is locally-created objects, not
objects obtained from other remotes.  Objects obtained from other
remotes are more of an edge case.

Thanks for your thoughtful comments.

Jonathan

Re: Partial clone design (with connectivity check for locally-created objects)

[Ben Peart]

On 8/4/2017 8:21 PM, Jonathan Tan wrote:

On Fri, 04 Aug 2017 15:51:08 -0700
Junio C Hamano  wrote:


Jonathan Tan  writes:


"Imported" objects must be in a packfile that has a ".remote"
file with arbitrary text (similar to the ".keep" file). They come from
clones, fetches, and the object loader (see below).
...
A "homegrown" object is valid if each object it references:
  1. is a "homegrown" object,
  2. is an "imported" object, or
  3. is referenced by an "imported" object.


Overall it captures what was discussed, and I think it is a good
start.


I missed the offline discussion and so am trying to piece together what 
this latest design is trying to do.  Please let me know if I'm not 
understanding something correctly.


From what I can tell, objects are going to be segmented into two 
"types" - those that were fetched from a remote source that allows 
partial clones/fetches (lazyobject/imported) and those that come from 
"regular" remote sources (homegrown) that requires all objects to exist 
locally.


FWIW, the names here are not making things clearer for me. If I'm 
correct perhaps "partial" and "normal" would be better to indicate the 
type of the source? Anyway...


Once the objects are segmented into the 2 types, the fsck connectivity 
check code is updated to ignore missing objects from "partial" remotes 
but still expect/validate them from "normal" remotes.


This compromise seems reasonable - don't generate errors for missing 
objects for remotes that returned a partial clone but do generate errors 
for missing objects from normal clones as a missing object is always an 
error in this case.


This segmentation is what is driving the need for the object loader to 
build a new local pack file for every command that has to fetch a 
missing object.  For example, we can't just write a tree object from a 
"partial" clone into the loose object store as we have no way for fsck 
to treat them differently and ignore any missing objects referenced by 
that tree object.


My concern with this proposal is the combination of 1) writing a new 
pack file for every git command that ends up bringing down a missing 
object and 2) gc not compressing those pack files into a single pack file.


We all know that git doesn't scale well with a lot of pack files as it 
has to do a linear search through all the pack files when attempting to 
find an object.  I can see that very quickly, there would be a lot of 
pack files generated and with gc ignoring "partial" pack files, this 
would never get corrected.


In our usage scenarios, _all_ of the objects come from "partial" clones 
so all of our objects would end up in a series of "partial" pack files 
and would have pretty poor performance as a result.


I wondered if it is possible to flag a specific remote as "partial" and 
have fsck be able to track any given object back to the remote and then 
properly handle the fact that it was missing based on that. I couldn't 
think of a good way to do that without some additional data structure 
that would have to be build/maintained (ie promises).


That thinking did lead me back to wondering again if we could live with 
a repo specific flag.  If any clone/fetch was "partial" the flag is set 
and fsck ignore missing objects whether they came from a "partial" 
remote or not.


I'll admit it isn't as robust if someone is mixing and matching remotes 
from different servers some of which are partial and some of which are 
not.  I'm not sure how often that would actually happen but I _am_ 
certain a single repo specific flag is a _much_ simpler model than 
anything else we've come up with so far.




I doubt you want to treat all fetches/clones the same way as the
"lazy object" loading, though.  You may be critically rely on the
corporate central server that will give the objects it "promised"
when you cloned from it lazily (i.e. it may have given you a commit,
but not its parents or objects contained in its tree--you still know
that the parents and the tree and its contents will later be
available and rely on that fact).  You trust that and build on top,
so the packfile you obtained when you cloned from such a server
should count as "imported".  But if you exchanged wip changes with
your colleages by fetching or pushing peer-to-peer, without the
corporate central server knowing, you would want to treat objects in
packs (or loose objects) you obtained that way as "not imported".


That's true. I discussed this with a teammate and we might need to make
extensions.lazyObject be the name of the "corporate central server"
remote instead, and have a "loader" setting within that remote, so that
we can distinguish that objects from this server are "imported" but
objects from other servers are not.

The connectivity check shouldn't be slow in this case because fetches
are usually onto tips that we have (so we don't hit case 3).


Also I think "imported" vs "homegrown" may be a bit misnomer; 

Re: [PATCH 0/6] clean up parsing of maybe_bool

[Stefan Beller]

On Mon, Aug 7, 2017 at 11:20 AM, Martin Ågren  wrote:
> When we want to parse a boolean config item without dying on error, we
> call git_config_maybe_bool() which takes two arguments: the value to be
> parsed (obviously) and a `name` which is completely ignored. Junio has
> suggested to drop `name` and rename the function [1]. That effort even
> started shortly after that, by introducing git_parse_maybe_bool(). The
> new function currently only has a single user outside config.c.
>
> Patch 5 of this series deprecates the old function and updates all
> callers to use git_parse_maybe_bool() instead. Patch 6 is a final
> cleanup: one of the converted callers suddenly had an unused argument.
>
> Patches 3 and 4 prepare for the switch. In particular, patch 4 ensures
> that the two functions are actually equivalent. In doing so, it affects
> `git push --signed=..` which was very slightly inconsistent to the rest
> of Git.
>
> Patch 2 adds a failing test in preparation for patch 4. Patch 1 corrects
> the documentation not to say "git push --sign=.." to make it a bit more
> obvious that the opposite typo is not being made in patches 2 and 4.
>
> git_parse_maybe_bool is used in sb/diff-color-move, which is in "next".
> This series makes --color-moved and diff.colormoved consistent with
> other booleans. That should be a good thing, but cc Stefan to be sure.

The series looks fine to me overall, though patch 5 is overly gentle IMHO.
We could have removed it right there as Junio is very good at resolving
conflicts or producing dirty merges for such a situation.
But delaying it until no other series' are in flight is fine with me, too.

Looking back at sb/diff-color-move and the code where
git_parse_maybe_bool is used, I do not think this will be an issue.

Thanks,
Stefan

Re: [PATCH] Fix delta integer overflows

[Junio C Hamano]

Martin Koegler  writes:

> From: Martin Koegler 
>
> The current delta code produces incorrect pack objects for files > 4GB.
>
> Signed-off-by: Martin Koegler 
> ---
>  diff-delta.c | 23 ---
>  1 file changed, 12 insertions(+), 11 deletions(-)
>
> Just pass any file > 4 GB to the delta-compression [by increasing the delta 
> limits].
> As file size, a truncated 32bit value will be encoded, leading to broken pack 
> files.

The patch obviously makes the code better and self consistent in
that "struct delta_index" has src_size as ulong, and this function
takes trg_size as ulong, and it was plain wrong for the code to
assume that "i", which is uint, can receive it safely.

In the longer term we might want to move to size_t or even
uintmax_t, as the ulong on a platform may not be long enough in
order to express the largest file size the platform can have, but
this patch (1) is good even without such a change, and (2) gives a
good foundation to build on if we want such a change on top.

Thanks.  Will queue.

>
> diff --git a/diff-delta.c b/diff-delta.c
> index 3797ce6..13e5a01 100644
> --- a/diff-delta.c
> +++ b/diff-delta.c
> @@ -319,7 +319,8 @@ create_delta(const struct delta_index *index,
>const void *trg_buf, unsigned long trg_size,
>unsigned long *delta_size, unsigned long max_size)
>  {
> - unsigned int i, outpos, outsize, moff, msize, val;
> + unsigned int i, val;
> + unsigned long l, outpos, outsize, moff, msize;
>   int inscnt;
>   const unsigned char *ref_data, *ref_top, *data, *top;
>   unsigned char *out;
> @@ -336,20 +337,20 @@ create_delta(const struct delta_index *index,
>   return NULL;
>  
>   /* store reference buffer size */
> - i = index->src_size;
> - while (i >= 0x80) {
> - out[outpos++] = i | 0x80;
> - i >>= 7;
> + l = index->src_size;
> + while (l >= 0x80) {
> + out[outpos++] = l | 0x80;
> + l >>= 7;
>   }
> - out[outpos++] = i;
> + out[outpos++] = l;
>  
>   /* store target buffer size */
> - i = trg_size;
> - while (i >= 0x80) {
> - out[outpos++] = i | 0x80;
> - i >>= 7;
> + l = trg_size;
> + while (l >= 0x80) {
> + out[outpos++] = l | 0x80;
> + l >>= 7;
>   }
> - out[outpos++] = i;
> + out[outpos++] = l;
>  
>   ref_data = index->src_buf;
>   ref_top = ref_data + index->src_size;

Re: reftable [v6]: new ref storage format

[Shawn Pearce]

On Mon, Aug 7, 2017 at 11:27 AM, Stefan Beller  wrote:
> On Sun, Aug 6, 2017 at 6:47 PM, Shawn Pearce  wrote:
>> 6th iteration of the reftable storage format.
>>
>> You can read a rendered version of this here:
>> https://googlers.googlesource.com/sop/jgit/+/reftable/Documentation/technical/reftable.md
>>
>> The index may be organized into a multi-level index, where ...
>> which may in turn point to either index blocks (3rd level) or ref blocks 
>> (leaf level).
>
> So we allow 3 levels at most?

No, its just an example. Large ref sets with small block size need 4
levels. Or more.

> The file format structure marks the indexes '?', should that be
> rather '*' to indicate there can be more than one index block?

Will fix in the next respin of the document, thanks.

Re: [PATCH for NEXT v3 2/2] sub-process: refactor handshake to common function

[Ben Peart]

On 8/7/2017 2:17 PM, Jonathan Tan wrote:

On Mon, 7 Aug 2017 19:51:04 +0200
Lars Schneider  wrote:




On 07 Aug 2017, at 19:21, Jonathan Tan  wrote:

On Sun, 6 Aug 2017 21:58:24 +0200
Lars Schneider  wrote:


+   struct cmd2process *entry = (struct cmd2process *)subprocess;
+   return subprocess_handshake(subprocess, "git-filter", versions, NULL,
+   capabilities,
+   >supported_capabilities);


Wouldn't it make sense to add `supported_capabilities` to `struct 
subprocess_entry` ?


The members of "struct subprocess_entry" are not supposed to be accessed
directly, according to the documentation. If we relaxed that, then we
could do this, but before that I think it's better to let the caller
handle it.


@Ben: You wrote that " Members should not be accessed directly.":
https://github.com/git/git/commit/99605d62e8e7e568035dc953b24b79b3d52f0522#diff-c1655ad5d68943a3dc5bfae8c98466f2R22
Can you give me a hint why?



It's just good object oriented design of providing a layer of 
abstraction between the implementation details and the use of the 
class/object/API.  I was following the model in api-hashmap.txt but 
there are many other examples of where we don't do this.


Perhaps providing a function that returns the property you want to 
access (similar to subprocess_get_child_process) would work.



@Jonathan: What do you mean by "it's better to let the caller handle it"


Let the caller provide their own place to store the capabilities, I
mean, instead of (say) using a field as you describe and an accessor
method.

I don't feel strongly about this, though.


It does, but so does chosen_version. This is meant to allow the caller
to pass NULL to this function.


Hm. I think every protocol should be versioned otherwise we could run
into trouble in the long run.

TBH I wouldn't support NULL in that case in the first place. If you
want to support it then I think we should document it.


Note that this NULL is for the chosen version as chosen by the server,
not the versions declared as supported by the client.

The protocol is versioned. Some users (e.g. the filter mechanism) of
this subprocess thing would want to pass NULL because they only support
one version and the subprocess thing already ensures that the server
report that it supports one of the versions sent.

Re: reftable [v6]: new ref storage format

[Stefan Beller]

On Sun, Aug 6, 2017 at 6:47 PM, Shawn Pearce  wrote:
> 6th iteration of the reftable storage format.
>
> You can read a rendered version of this here:
> https://googlers.googlesource.com/sop/jgit/+/reftable/Documentation/technical/reftable.md
>
> Changes from v5:
> - extensions.refStorage = reftable is used to select this format.
>
> - Log records can be explicitly deleted (for refs/stash).
> - Log records may use Michael Haggerty's chained idea to compress before zlib.
>   This saved ~5.8% on one of my example repositories.

Some observations:

Also the bits in the records changed in v5 or v6:
  0x0..0x3 is valid for a ref,
  obj records have a ccnt
  0x0, 0x1, 0x4..0x7 are used in the logs

We have the following block indicators:
  'r'  ref block
  'o' object block
  'g' log block

  high bit for any index.

Without prior knowledge an index doesn't indicate if it
indexes refs, objects or logs. To find out, one must follow
an arbitrary entry which points to either an index again
or at a block marked with 'r', 'o' or 'g'.

Okay with me.

> The index may be organized into a multi-level index, where ...
> which may in turn point to either index blocks (3rd level) or ref blocks 
> (leaf level).

So we allow 3 levels at most?

The file format structure marks the indexes '?', should that be
rather '*' to indicate there can be more than one index block?

[PATCH 1/6] Doc/git-{push,send-pack}: correct --sign= to --signed=

[Martin Ågren]

Since we're about to touch the behavior of --signed=, do this as a
preparatory step.

The documentation mentions --sign=, and it works. But that's just
because it's an unambiguous abbreviation of --signed, which is how it is
actually implemented. This was added in commit 30261094 ("push: support
signing pushes iff the server supports it", 2015-08-19). Back when that
series was developed [1] [2], there were suggestions about both --sign=
and --signed=. The final implementation settled on --signed=, but some
of the documentation and commit messages ended up using --sign=.

The option is referred to as --signed= in Documentation/config.txt
(under push.gpgSign).

One could argue that we have promised --sign for two years now, so we
should implement it as an alias for --signed. (Then we might also
deprecate the latter, something which was considered already then.) That
would be a slightly more intrusive change.

This minor issue would only be a problem once we want to implement some
other option --signfoo, but the earlier we do this step, the better.

[1] v1-thread:
https://public-inbox.org/git/1439492451-11233-1-git-send-email-dborow...@google.com/T/#u

[2] v2-thread:
https://public-inbox.org/git/1439998007-28719-1-git-send-email-dborow...@google.com/T/#m6533a6c4707a30b0d81e86169ff8559460cbf6eb

Signed-off-by: Martin Ågren 
---
 Documentation/git-push.txt  | 4 ++--
 Documentation/git-send-pack.txt | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 0a639664f..3e76e99f3 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -12,7 +12,7 @@ SYNOPSIS
 'git push' [--all | --mirror | --tags] [--follow-tags] [--atomic] [-n | 
--dry-run] [--receive-pack=]
   [--repo=] [-f | --force] [-d | --delete] [--prune] [-v | 
--verbose]
   [-u | --set-upstream] [--push-option=]
-  [--[no-]signed|--sign=(true|false|if-asked)]
+  [--[no-]signed|--signed=(true|false|if-asked)]
   [--force-with-lease[=[:]]]
   [--no-verify] [ [...]]
 
@@ -141,7 +141,7 @@ already exists on the remote side.
information, see `push.followTags` in linkgit:git-config[1].
 
 --[no-]signed::
---sign=(true|false|if-asked)::
+--signed=(true|false|if-asked)::
GPG-sign the push request to update refs on the receiving
side, to allow it to be checked by the hooks and/or be
logged.  If `false` or `--no-signed`, no signing will be
diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt
index 966abb0df..f51c64939 100644
--- a/Documentation/git-send-pack.txt
+++ b/Documentation/git-send-pack.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 [verse]
 'git send-pack' [--all] [--dry-run] [--force] 
[--receive-pack=]
[--verbose] [--thin] [--atomic]
-   [--[no-]signed|--sign=(true|false|if-asked)]
+   [--[no-]signed|--signed=(true|false|if-asked)]
[:] [...]
 
 DESCRIPTION
@@ -71,7 +71,7 @@ be in a separate packet, and the list must end with a flush 
packet.
refs.
 
 --[no-]signed::
---sign=(true|false|if-asked)::
+--signed=(true|false|if-asked)::
GPG-sign the push request to update refs on the receiving
side, to allow it to be checked by the hooks and/or be
logged.  If `false` or `--no-signed`, no signing will be
-- 
2.14.0.5.g0f7b1ed27

[PATCH 0/6] clean up parsing of maybe_bool

[Martin Ågren]

When we want to parse a boolean config item without dying on error, we
call git_config_maybe_bool() which takes two arguments: the value to be
parsed (obviously) and a `name` which is completely ignored. Junio has
suggested to drop `name` and rename the function [1]. That effort even
started shortly after that, by introducing git_parse_maybe_bool(). The
new function currently only has a single user outside config.c.

Patch 5 of this series deprecates the old function and updates all
callers to use git_parse_maybe_bool() instead. Patch 6 is a final
cleanup: one of the converted callers suddenly had an unused argument.

Patches 3 and 4 prepare for the switch. In particular, patch 4 ensures
that the two functions are actually equivalent. In doing so, it affects
`git push --signed=..` which was very slightly inconsistent to the rest
of Git.

Patch 2 adds a failing test in preparation for patch 4. Patch 1 corrects
the documentation not to say "git push --sign=.." to make it a bit more
obvious that the opposite typo is not being made in patches 2 and 4.

git_parse_maybe_bool is used in sb/diff-color-move, which is in "next".
This series makes --color-moved and diff.colormoved consistent with
other booleans. That should be a good thing, but cc Stefan to be sure.

Martin

[1] https://public-inbox.org/git/xmqq7fotd71o@gitster.dls.corp.google.com/

Martin Ågren (6):
  Doc/git-{push,send-pack}: correct --sign= to --signed=
  t5334: document that git push --signed=1 does not work
  config: introduce git_parse_maybe_bool_text
  config: make git_{config,parse}_maybe_bool equivalent
  treewide: deprecate git_config_maybe_bool, use git_parse_maybe_bool
  parse_decoration_style: drop unused argument `var`

 Documentation/git-push.txt |  4 ++--
 Documentation/git-send-pack.txt|  4 ++--
 Documentation/technical/api-config.txt |  4 
 t/t5534-push-signed.sh |  7 +++
 builtin/log.c  | 10 +-
 builtin/merge.c|  4 ++--
 builtin/pull.c |  4 ++--
 builtin/push.c |  2 +-
 builtin/remote.c   |  2 +-
 builtin/send-pack.c|  2 +-
 config.c   | 15 ++-
 pager.c|  2 +-
 submodule-config.c |  6 +++---
 13 files changed, 41 insertions(+), 25 deletions(-)

-- 
2.14.0.5.g0f7b1ed27

[PATCH 3/6] config: introduce git_parse_maybe_bool_text

[Martin Ågren]

Commit 9a549d43 ("config.c: rename git_config_maybe_bool_text and export
it as git_parse_maybe_bool", 2015-08-19) intended git_parse_maybe_bool
to be a replacement for git_config_maybe_bool, which could then be
retired. That is not obvious from the commit message, but that is what
the background on the mailing list suggests [1].

However, git_{config,parse}_maybe_bool do not handle all input the same.
Before the rename, that was by design and there is a caller in config.c
which requires git_parse_maybe_bool to behave exactly as it does.

Prepare for the next patch by renaming git_parse_maybe_bool to ..._text
and reimplementing the first one as a simple call to the second one. Let
the existing users in config.c use ..._text, since it does what they
need.

[1] https://public-inbox.org/git/xmqq7fotd71o@gitster.dls.corp.google.com/

Signed-off-by: Martin Ågren 
---
 config.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/config.c b/config.c
index 231f9a750..7df57cec0 100644
--- a/config.c
+++ b/config.c
@@ -928,7 +928,7 @@ ssize_t git_config_ssize_t(const char *name, const char 
*value)
return ret;
 }
 
-int git_parse_maybe_bool(const char *value)
+static int git_parse_maybe_bool_text(const char *value)
 {
if (!value)
return 1;
@@ -945,9 +945,14 @@ int git_parse_maybe_bool(const char *value)
return -1;
 }
 
+int git_parse_maybe_bool(const char *value)
+{
+   return git_parse_maybe_bool_text(value);
+}
+
 int git_config_maybe_bool(const char *name, const char *value)
 {
-   int v = git_parse_maybe_bool(value);
+   int v = git_parse_maybe_bool_text(value);
if (0 <= v)
return v;
if (git_parse_int(value, ))
@@ -957,7 +962,7 @@ int git_config_maybe_bool(const char *name, const char 
*value)
 
 int git_config_bool_or_int(const char *name, const char *value, int *is_bool)
 {
-   int v = git_parse_maybe_bool(value);
+   int v = git_parse_maybe_bool_text(value);
if (0 <= v) {
*is_bool = 1;
return v;
-- 
2.14.0.5.g0f7b1ed27

[PATCH 5/6] treewide: deprecate git_config_maybe_bool, use git_parse_maybe_bool

[Martin Ågren]

The only difference between these is that the former takes an argument
`name` which it ignores completely. Still, the callers are quite careful
to provide reasonable values for it.

Once in-flight topics have landed, we should be able to remove
git_config_maybe_bool. In the meantime, document it as deprecated in the
technical documentation. While at it, document git_parse_maybe_bool.

Signed-off-by: Martin Ågren 
---
 Documentation/technical/api-config.txt | 4 
 builtin/log.c  | 4 ++--
 builtin/merge.c| 4 ++--
 builtin/pull.c | 4 ++--
 builtin/push.c | 2 +-
 builtin/remote.c   | 2 +-
 builtin/send-pack.c| 2 +-
 config.c   | 2 +-
 pager.c| 2 +-
 submodule-config.c | 6 +++---
 10 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/Documentation/technical/api-config.txt 
b/Documentation/technical/api-config.txt
index 20741f345..7a83a3a6e 100644
--- a/Documentation/technical/api-config.txt
+++ b/Documentation/technical/api-config.txt
@@ -187,6 +187,10 @@ Same as `git_config_bool`, except that integers are 
returned as-is, and
 an `is_bool` flag is unset.
 
 `git_config_maybe_bool`::
+Deprecated. Use `git_parse_maybe_bool` instead. They are exactly the
+same, except this function takes an unused argument `name`.
+
+`git_parse_maybe_bool`::
 Same as `git_config_bool`, except that it returns -1 on error rather
 than dying.
 
diff --git a/builtin/log.c b/builtin/log.c
index c6362cf92..9182f0ee3 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -60,7 +60,7 @@ static int auto_decoration_style(void)
 
 static int parse_decoration_style(const char *var, const char *value)
 {
-   switch (git_config_maybe_bool(var, value)) {
+   switch (git_parse_maybe_bool(value)) {
case 1:
return DECORATE_SHORT_REFS;
case 0:
@@ -821,7 +821,7 @@ static int git_format_config(const char *var, const char 
*value, void *cb)
return 0;
}
if (!strcmp(var, "format.from")) {
-   int b = git_config_maybe_bool(var, value);
+   int b = git_parse_maybe_bool(value);
free(from);
if (b < 0)
from = xstrdup(value);
diff --git a/builtin/merge.c b/builtin/merge.c
index 900bafdb4..6a5122921 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -566,7 +566,7 @@ static int git_merge_config(const char *k, const char *v, 
void *cb)
else if (!strcmp(k, "merge.renormalize"))
option_renormalize = git_config_bool(k, v);
else if (!strcmp(k, "merge.ff")) {
-   int boolval = git_config_maybe_bool(k, v);
+   int boolval = git_parse_maybe_bool(v);
if (0 <= boolval) {
fast_forward = boolval ? FF_ALLOW : FF_NO;
} else if (v && !strcmp(v, "only")) {
@@ -940,7 +940,7 @@ static int default_edit_option(void)
return 0;
 
if (e) {
-   int v = git_config_maybe_bool(name, e);
+   int v = git_parse_maybe_bool(e);
if (v < 0)
die(_("Bad value '%s' in environment '%s'"), e, name);
return v;
diff --git a/builtin/pull.c b/builtin/pull.c
index 9b86e519b..7fe281414 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -39,7 +39,7 @@ enum rebase_type {
 static enum rebase_type parse_config_rebase(const char *key, const char *value,
int fatal)
 {
-   int v = git_config_maybe_bool("pull.rebase", value);
+   int v = git_parse_maybe_bool(value);
 
if (!v)
return REBASE_FALSE;
@@ -274,7 +274,7 @@ static const char *config_get_ff(void)
if (git_config_get_value("pull.ff", ))
return NULL;
 
-   switch (git_config_maybe_bool("pull.ff", value)) {
+   switch (git_parse_maybe_bool(value)) {
case 0:
return "--no-ff";
case 1:
diff --git a/builtin/push.c b/builtin/push.c
index 03846e837..2ac810422 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -481,7 +481,7 @@ static int git_push_config(const char *k, const char *v, 
void *cb)
} else if (!strcmp(k, "push.gpgsign")) {
const char *value;
if (!git_config_get_value("push.gpgsign", )) {
-   switch (git_config_maybe_bool("push.gpgsign", value)) {
+   switch (git_parse_maybe_bool(value)) {
case 0:
set_push_cert_flags(flags, 
SEND_PACK_PUSH_CERT_NEVER);
break;
diff --git a/builtin/remote.c b/builtin/remote.c
index 6273c0c23..a995ea86c 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -301,7 +301,7 @@ static int config_read_branches(const char *key, const char 

[PATCH 2/6] t5334: document that git push --signed=1 does not work

[Martin Ågren]

When accepting booleans as command-line or config options throughout
Git, there are several documented synonyms for true and false.
However, one particular user is slightly broken: `git push --signed=..`
does not understand the integer synonyms for true and false.

This is hardly wanted. The --signed option has a different notion of
boolean than all other arguments and config options, including the
config option corresponding to it, push.gpgSign.

Add a test documenting the failure to handle --signed=1.

Signed-off-by: Martin Ågren 
---
 t/t5534-push-signed.sh | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
index 464ffdd14..5dce55e1d 100755
--- a/t/t5534-push-signed.sh
+++ b/t/t5534-push-signed.sh
@@ -71,6 +71,13 @@ test_expect_success 'push --signed fails with a receiver 
without push certificat
test_i18ngrep "the receiving end does not support" err
 '
 
+test_expect_failure 'push --signed=1 is accepted' '
+   prepare_dst &&
+   mkdir -p dst/.git/hooks &&
+   test_must_fail git push --signed=1 dst noop ff +noff 2>err &&
+   test_i18ngrep "the receiving end does not support" err
+'
+
 test_expect_success GPG 'no certificate for a signed push with no update' '
prepare_dst &&
mkdir -p dst/.git/hooks &&
-- 
2.14.0.5.g0f7b1ed27

[PATCH 4/6] config: make git_{config,parse}_maybe_bool equivalent

[Martin Ågren]

Both of these act on a string `value` which they parse as a boolean. The
"parse"-variant was introduced as a replacement for the "config"-variant
which for historical reasons takes an unused argument `name`. That it
was intended as a replacement is not obvious from commit 9a549d43
("config.c: rename git_config_maybe_bool_text and export it as
git_parse_maybe_bool", 2015-08-19), but that is what the background on
the mailing list suggests [1].

However, these two functions do not parse `value` in exactly the same
way. In particular, git_config_maybe_bool accepts integers (0 for false,
non-0 for true). This means there are two slightly different definitions
of "maybe_bool" in the code-base, and that every time a call to
git_config_maybe_bool is changed to use git_parse_maybe_bool, it risks
breaking someone's workflow.

Move the implementation of "config" into "parse" and make the latter a
trivial wrapper.

This also fixes the only user of git_parse_maybe_bool, `git push
--signed=..`.

[1] https://public-inbox.org/git/xmqq7fotd71o@gitster.dls.corp.google.com/

Signed-off-by: Martin Ågren 
---
 t/t5534-push-signed.sh |  2 +-
 config.c   | 10 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
index 5dce55e1d..1cea758f7 100755
--- a/t/t5534-push-signed.sh
+++ b/t/t5534-push-signed.sh
@@ -71,7 +71,7 @@ test_expect_success 'push --signed fails with a receiver 
without push certificat
test_i18ngrep "the receiving end does not support" err
 '
 
-test_expect_failure 'push --signed=1 is accepted' '
+test_expect_success 'push --signed=1 is accepted' '
prepare_dst &&
mkdir -p dst/.git/hooks &&
test_must_fail git push --signed=1 dst noop ff +noff 2>err &&
diff --git a/config.c b/config.c
index 7df57cec0..d87376a5d 100644
--- a/config.c
+++ b/config.c
@@ -946,11 +946,6 @@ static int git_parse_maybe_bool_text(const char *value)
 }
 
 int git_parse_maybe_bool(const char *value)
-{
-   return git_parse_maybe_bool_text(value);
-}
-
-int git_config_maybe_bool(const char *name, const char *value)
 {
int v = git_parse_maybe_bool_text(value);
if (0 <= v)
@@ -960,6 +955,11 @@ int git_config_maybe_bool(const char *name, const char 
*value)
return -1;
 }
 
+int git_config_maybe_bool(const char *name, const char *value)
+{
+   return git_parse_maybe_bool(value);
+}
+
 int git_config_bool_or_int(const char *name, const char *value, int *is_bool)
 {
int v = git_parse_maybe_bool_text(value);
-- 
2.14.0.5.g0f7b1ed27

[PATCH 6/6] parse_decoration_style: drop unused argument `var`

[Martin Ågren]

The previous commit left it unused.

Signed-off-by: Martin Ågren 
---
 builtin/log.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 9182f0ee3..483d15a94 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -58,7 +58,7 @@ static int auto_decoration_style(void)
return (isatty(1) || pager_in_use()) ? DECORATE_SHORT_REFS : 0;
 }
 
-static int parse_decoration_style(const char *var, const char *value)
+static int parse_decoration_style(const char *value)
 {
switch (git_parse_maybe_bool(value)) {
case 1:
@@ -82,7 +82,7 @@ static int decorate_callback(const struct option *opt, const 
char *arg, int unse
if (unset)
decoration_style = 0;
else if (arg)
-   decoration_style = parse_decoration_style("command line", arg);
+   decoration_style = parse_decoration_style(arg);
else
decoration_style = DECORATE_SHORT_REFS;
 
@@ -409,7 +409,7 @@ static int git_log_config(const char *var, const char 
*value, void *cb)
if (!strcmp(var, "log.date"))
return git_config_string(_date_mode, var, value);
if (!strcmp(var, "log.decorate")) {
-   decoration_style = parse_decoration_style(var, value);
+   decoration_style = parse_decoration_style(value);
if (decoration_style < 0)
decoration_style = 0; /* maybe warn? */
return 0;
-- 
2.14.0.5.g0f7b1ed27

Re: [PATCH] Drop some dashes from built-in invocations in scripts

[Junio C Hamano]

Michael Forney  writes:

> However, I still think the patch should be applied for
> self-consistency at least (git-submodule.sh currently calls both `git
> rev-parse` and `git-rev-parse`).

Oh, there is no question about the changes in the patch being good,
as I already said.  We want to make sure that people who copy &
paste code would see fewer instances of "git-foo".

I was commenting on the justification in your proposed log message,
which I realized was a bit misleading, after deciding that the patch
text itself is good and I need to apply it.

Thanks for the clean-up.

Re: [PATCH for NEXT v3 2/2] sub-process: refactor handshake to common function

[Jonathan Tan]

On Mon, 7 Aug 2017 19:51:04 +0200
Lars Schneider  wrote:

> 
> > On 07 Aug 2017, at 19:21, Jonathan Tan  wrote:
> > 
> > On Sun, 6 Aug 2017 21:58:24 +0200
> > Lars Schneider  wrote:
> > 
> >>> + struct cmd2process *entry = (struct cmd2process *)subprocess;
> >>> + return subprocess_handshake(subprocess, "git-filter", versions, NULL,
> >>> + capabilities,
> >>> + >supported_capabilities);
> >> 
> >> Wouldn't it make sense to add `supported_capabilities` to `struct 
> >> subprocess_entry` ?
> > 
> > The members of "struct subprocess_entry" are not supposed to be accessed
> > directly, according to the documentation. If we relaxed that, then we
> > could do this, but before that I think it's better to let the caller
> > handle it.
> 
> @Ben: You wrote that " Members should not be accessed directly.":
> https://github.com/git/git/commit/99605d62e8e7e568035dc953b24b79b3d52f0522#diff-c1655ad5d68943a3dc5bfae8c98466f2R22
> Can you give me a hint why?
> 
> @Jonathan: What do you mean by "it's better to let the caller handle it"

Let the caller provide their own place to store the capabilities, I
mean, instead of (say) using a field as you describe and an accessor
method.

I don't feel strongly about this, though.

> > It does, but so does chosen_version. This is meant to allow the caller
> > to pass NULL to this function.
> 
> Hm. I think every protocol should be versioned otherwise we could run
> into trouble in the long run.
> 
> TBH I wouldn't support NULL in that case in the first place. If you
> want to support it then I think we should document it.

Note that this NULL is for the chosen version as chosen by the server,
not the versions declared as supported by the client.

The protocol is versioned. Some users (e.g. the filter mechanism) of
this subprocess thing would want to pass NULL because they only support
one version and the subprocess thing already ensures that the server
report that it supports one of the versions sent.

[PATCH] Fix delta integer overflows

[Martin Koegler]

From: Martin Koegler 

The current delta code produces incorrect pack objects for files > 4GB.

Signed-off-by: Martin Koegler 
---
 diff-delta.c | 23 ---
 1 file changed, 12 insertions(+), 11 deletions(-)

Just pass any file > 4 GB to the delta-compression [by increasing the delta 
limits].
As file size, a truncated 32bit value will be encoded, leading to broken pack 
files.

diff --git a/diff-delta.c b/diff-delta.c
index 3797ce6..13e5a01 100644
--- a/diff-delta.c
+++ b/diff-delta.c
@@ -319,7 +319,8 @@ create_delta(const struct delta_index *index,
 const void *trg_buf, unsigned long trg_size,
 unsigned long *delta_size, unsigned long max_size)
 {
-   unsigned int i, outpos, outsize, moff, msize, val;
+   unsigned int i, val;
+   unsigned long l, outpos, outsize, moff, msize;
int inscnt;
const unsigned char *ref_data, *ref_top, *data, *top;
unsigned char *out;
@@ -336,20 +337,20 @@ create_delta(const struct delta_index *index,
return NULL;
 
/* store reference buffer size */
-   i = index->src_size;
-   while (i >= 0x80) {
-   out[outpos++] = i | 0x80;
-   i >>= 7;
+   l = index->src_size;
+   while (l >= 0x80) {
+   out[outpos++] = l | 0x80;
+   l >>= 7;
}
-   out[outpos++] = i;
+   out[outpos++] = l;
 
/* store target buffer size */
-   i = trg_size;
-   while (i >= 0x80) {
-   out[outpos++] = i | 0x80;
-   i >>= 7;
+   l = trg_size;
+   while (l >= 0x80) {
+   out[outpos++] = l | 0x80;
+   l >>= 7;
}
-   out[outpos++] = i;
+   out[outpos++] = l;
 
ref_data = index->src_buf;
ref_top = ref_data + index->src_size;
-- 
2.1.4

Re: [PATCH v1] am: fix signoff when other trailers are present

[Jonathan Tan]

On Mon, 07 Aug 2017 10:49:28 -0700
Junio C Hamano  wrote:

> Phillip Wood  writes:
> 
> > From: Phillip Wood 
> >
> > If there was no 'Signed-off-by:' trailer but another trailer such as
> > 'Reported-by:' then 'git am --signoff' would add a blank line between
> > the existing trailers and the added 'Signed-off-by:' line. e.g.
> >
> > Rebase accepts '--rerere-autoupdate' as an option but only honors
> > it if '-m' is also given. Fix it for a non-interactive rebase by
> > passing on the option to 'git am' and 'git cherry-pick'.
> >
> > Reported-by: Junio C Hamano 
> >
> > Signed-off-by: Phillip Wood 
> >
> > Fix by using the code provided for this purpose in sequencer.c.
> > Change the tests so that they check the formatting of the
> > 'Signed-off-by:' lines rather than just grepping for them.
> >
> > Signed-off-by: Phillip Wood 
> > ---
> > I'm not sure if this should be calling ignore_non_trailer() or not -
> > git commit does but git cherry-pick does not. This follows commit and
> > cherry-pick in ignoring the value of trailer.ifExists for the signoff.
> > I'm a bit surprised they do that - is it correct?
> 
> These built-in "sign-off" machinery long predates the "trailer"
> thing, so I am not surprised if they do not behave the same.  I
> vaguely recall having discussions on this earlier this year, but
> details escape me.  
> 
> Asking Jonathan, who did a series that ends at 44dc738a ("sequencer:
> add newline before adding footers", 2017-04-26), and Christian, who
> is the original contirbutor to the "trailer" machinery, for input.

Regarding ignore_non_trailer(), I believe that's because "git commit"
wants to tolerate blank lines and comments after the "real" commit
message, whereas "git cherry-pick" doesn't need to. As far as I can
tell, this "git am" case is similar to "git cherry-pick".

Regarding trailer.ifExists, the then existing behavior was to refrain
from writing a new sign-off line only if it would be a duplicate of the
last one, regardless of trailer.ifExists (as Junio says, back then, the
sign-off mechanism and the trailer mechanism were independent). I
preserved that behavior.

Re: [PATCH] tests: don't give unportable ">" to "test" built-in, use -gt

[Andreas Schwab]

On Aug 06 2017, Ævar Arnfjörð Bjarmason  wrote:

> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index 0ef7b94394..0e2e57aa3d 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -1887,7 +1887,7 @@ EOF"
>   run_with_limited_stack git tag --contains HEAD >actual &&
>   test_cmp expect actual &&
>   run_with_limited_stack git tag --no-contains HEAD >actual &&
> - test_line_count ">" 10 actual
> + test_line_count "-gt" 10 actual

Maybe also remove the quotes, they are no longer needed.

Andreas.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Drop some dashes from built-in invocations in scripts

[Michael Forney]

On 8/7/17, Junio C Hamano  wrote:
> Just to avoid possible confusion, the above is not to say "once it
> is decided, you are not allowed to bring fresh arguments to the
> discussion".  As Peff said [*2*] in that old discussion thread, the
> circumstances may have changed over 9 years, and it may benefit to
> revisit some old decisions.
>
> So in that sense, I do not mind somebody makes a fresh proposal,
> which would probably be similar to what I did back then in [*3*],
> which is at the beginning of that old thread.  But I do not think I
> would be doing so myself, and I suspect that I would not be very
> supportive for such a proposal, because my gut feeling is that the
> upside is not big enough compared to downsides.
>
> The obvious upside is that you do not have to have many built-in
> commands on the filesystem, either as a hardlink, a copy, or a
> symbolic link.  But we will be breaking people's scripts by breaking
> the 11-year old promise that we will keep their "git-foo" working as
> long as they prepend $GIT_EXEC_PATH to their $PATH we we did so.
> Another downside is that we now will expose which subcommands are
> built-in and which are not, which is unnecessarily implementation
> detail we'd want end-users to rely on.
>
> The "'git co' may stop working" I mentioned in my previous message
> is not counted as a downside---if the upside is large enough, I
> think that "we respawn git-foo as dashed built-in when running an
> alias that expands to 'foo'" can be fixed to respawn "git foo"
> instead of "git-foo".  But there may be other downside that we may
> not be able to fix, and I suspect that "we'd be breaking the 11-year
> old promise" is something we would not be able to fix.  That is why
> I doubt that I would be advocating the removal of "git-foo" from the
> filesystem myself.

Thanks for the history and explanation, Junio. I agree with your analysis.

I didn't know that git aliases invoke the `git-foo` path for built-ins
(I don't use them much myself, so didn't notice).

I also didn't know that it was supported to add GIT_EXEC_DIR to your
PATH to be able to call `git-foo`. I generally think of /libexec as
implementation-specific executables that a tool may call internally
(in that sense, whether or not the commands are built-ins would remain
an implementation-detail).

However, I still think the patch should be applied for
self-consistency at least (git-submodule.sh currently calls both `git
rev-parse` and `git-rev-parse`). Also, based on Johannes' reply, it
may still be useful for git-for-windows.

>
> [References]
>
> *1*
> https://public-inbox.org/git/alpine.lfd.1.10.0808261114070.3...@nehalem.linux-foundation.org/
>
> *2*
> https://public-inbox.org/git/20080826145719.gb5...@coredump.intra.peff.net/
>
> *3* https://public-inbox.org/git/7vprnzt7d5@gitster.siamese.dyndns.org/

Re: [PATCH for NEXT v3 2/2] sub-process: refactor handshake to common function

[Lars Schneider]

> On 07 Aug 2017, at 19:21, Jonathan Tan  wrote:
> 
> On Sun, 6 Aug 2017 21:58:24 +0200
> Lars Schneider  wrote:
> 
>>> +   struct cmd2process *entry = (struct cmd2process *)subprocess;
>>> +   return subprocess_handshake(subprocess, "git-filter", versions, NULL,
>>> +   capabilities,
>>> +   >supported_capabilities);
>> 
>> Wouldn't it make sense to add `supported_capabilities` to `struct 
>> subprocess_entry` ?
> 
> The members of "struct subprocess_entry" are not supposed to be accessed
> directly, according to the documentation. If we relaxed that, then we
> could do this, but before that I think it's better to let the caller
> handle it.

@Ben: You wrote that " Members should not be accessed directly.":
https://github.com/git/git/commit/99605d62e8e7e568035dc953b24b79b3d52f0522#diff-c1655ad5d68943a3dc5bfae8c98466f2R22
Can you give me a hint why?

@Jonathan: What do you mean by "it's better to let the caller handle it"


>>> 
>>> +static int handshake_version(struct child_process *process,
>>> +const char *welcome_prefix, int *versions,
>> 
>> Maybe it would be less ambiguous if we call it `supported_versions` ? 
> 
> I thought of that, but I think "supported_versions" is actually more
> ambiguous, since we don't know if these are versions supported by the
> server or client or both.

True! Maybe `versions_supported_by_git` to annoy people that hate 
long variable names ;-)


>>> +int *chosen_version)
>>> +{
>>> +   int version_scratch;
>>> +   int i;
>>> +   char *line;
>>> +   const char *p;
>>> +
>>> +   if (!chosen_version)
>>> +   chosen_version = _scratch;
>> 
>> I am not an C expert but wouldn't 'version_scratch' go out of scope as soon
>> as the function returns? Why don't you error here right away?
> 
> It does, but so does chosen_version. This is meant to allow the caller
> to pass NULL to this function.

Hm. I think every protocol should be versioned otherwise we could run
into trouble in the long run.

TBH I wouldn't support NULL in that case in the first place. If you
want to support it then I think we should document it.


>>> +   if (packet_write_fmt_gently(process->in, "%s-client\n",
>>> +   welcome_prefix))
>>> +   return error("Could not write client identification");
>> 
>> Nit: Would it make sense to rename `welcome_prefix` to `client_id`?
>> Alternatively, could we rename the error messages to "welcome prefix"?
> 
> I was retaining the existing terminology, but your suggestions seem
> reasonable. This might be best done in another patch once this series
> lands in master, though.

Yeah, sorry for my late review :-(


>>> +   for (i = 0; versions[i]; i++) {
>>> +   if (packet_write_fmt_gently(process->in, "version=%d\n",
>>> +   versions[i]))
>>> +   return error("Could not write requested version");
>> 
>> Maybe: "Could not write supported versions"?
> 
> Same as above - "supported" is ambiguous.
> 
>>> +   }
>>> +   if (packet_flush_gently(process->in))
>>> +   return error("Could not write flush packet");
>> 
>> I feel this error is too generic.
>> Maybe: "Could not finish writing supported versions"?
> 
> That's reasonable. This is a rare error, though, and if it does occur, I
> think this message is more informative. But I'm OK either way.

My thinking is this: if I see an error then I want to roughly know what
went wrong and I want to have a good chance to find the error in the
source. The "Could not write flush packet" is technically correct but
it makes it harder to pinpoint the error in the source as we throw
it in several places.


> 
>>> +
>>> +   if (!(line = packet_read_line(process->out, NULL)) ||
>>> +   !skip_prefix(line, welcome_prefix, ) ||
>>> +   strcmp(p, "-server"))
>>> +   return error("Unexpected line '%s', expected %s-server",
>>> +line ? line : "", welcome_prefix);
>>> +   if (!(line = packet_read_line(process->out, NULL)) ||
>>> +   !skip_prefix(line, "version=", ) ||
>>> +   strtol_i(p, 10, chosen_version))
>> 
>> Maybe `strlen("version=")` would be more clear than 10?
> 
> The 10 here is the base, not the length. If there's a better way to
> convert strings to integers, let me know.

Argh, of course! Sorry! To my defense: it was late last night :-)


> 
>>> +   return error("Unexpected line '%s', expected version",
>> 
>> Maybe "... expected version number" ?
> 
> I'm fine either way.
> 
>>> +static int handshake_capabilities(struct child_process *process,
>>> + struct subprocess_capability *capabilities,
>>> + unsigned int *supported_capabilities)
>> 
>> I feel the naming could be misleading. I think ...
>> `capabilities` is really `supported_capabilities` 
>> and 
>> 

Re: [PATCH v1] am: fix signoff when other trailers are present

[Junio C Hamano]

Phillip Wood  writes:

> From: Phillip Wood 
>
> If there was no 'Signed-off-by:' trailer but another trailer such as
> 'Reported-by:' then 'git am --signoff' would add a blank line between
> the existing trailers and the added 'Signed-off-by:' line. e.g.
>
> Rebase accepts '--rerere-autoupdate' as an option but only honors
> it if '-m' is also given. Fix it for a non-interactive rebase by
> passing on the option to 'git am' and 'git cherry-pick'.
>
> Reported-by: Junio C Hamano 
>
> Signed-off-by: Phillip Wood 
>
> Fix by using the code provided for this purpose in sequencer.c.
> Change the tests so that they check the formatting of the
> 'Signed-off-by:' lines rather than just grepping for them.
>
> Signed-off-by: Phillip Wood 
> ---
> I'm not sure if this should be calling ignore_non_trailer() or not -
> git commit does but git cherry-pick does not. This follows commit and
> cherry-pick in ignoring the value of trailer.ifExists for the signoff.
> I'm a bit surprised they do that - is it correct?

These built-in "sign-off" machinery long predates the "trailer"
thing, so I am not surprised if they do not behave the same.  I
vaguely recall having discussions on this earlier this year, but
details escape me.  

Asking Jonathan, who did a series that ends at 44dc738a ("sequencer:
add newline before adding footers", 2017-04-26), and Christian, who
is the original contirbutor to the "trailer" machinery, for input.

Re: [PATCH 4/4] imap-send: use curl by default

[Martin Ågren]

On 7 August 2017 at 19:10, Nicolas Morey-Chaisemartin
 wrote:
>
>
> Le 07/08/2017 à 18:37, Martin Ågren a écrit :
>> On 7 August 2017 at 16:04, Nicolas Morey-Chaisemartin
>>  wrote:
>>> Signed-off-by: Nicolas Morey-Chaisemartin 
>>> ---
>>>  imap-send.c | 6 --
>>>  1 file changed, 6 deletions(-)
>>>
>>> diff --git a/imap-send.c b/imap-send.c
>>> index 90b8683ed..4ebc16437 100644
>>> --- a/imap-send.c
>>> +++ b/imap-send.c
>>> @@ -35,13 +35,7 @@ typedef void *SSL;
>>>  #include "http.h"
>>>  #endif
>>>
>>> -#if defined(USE_CURL_FOR_IMAP_SEND) && defined(NO_OPENSSL)
>>> -/* only available option */
>>>  #define USE_CURL_DEFAULT 1
>>> -#else
>>> -/* strictly opt in */
>>> -#define USE_CURL_DEFAULT 0
>>> -#endif
>>>
>>>  static int verbosity;
>>>  static int use_curl = USE_CURL_DEFAULT;
>> So this is now basically "static int use_curl = 1;".
>>
>> Do we need a compile-time escape-hatch in case someone really needs
>> to avoid curl, e.g., because they have a too old version? I suppose
>> there is a conceptual difference between the "default", i.e., the value
>> of USE_CURL_DEFAULT that is assigned to "use_curl", and the "default
>> default", i.e., the value that is normally assigned to USE_CURL_DEFAULT.
>>
>> Martin
>
> The curl code depends on USE_CURL_FOR_IMAP_SEND so even with use_curl == 1, 
> it won't be an issue for people without curl (or old one).

I have just looked at the sources and haven't thought too hard about it,
but doesn't it mean that compiling without USE_CURL_FOR_IMAP_SEND
results in a binary such that you must use --no-curl or get used to seeing
"warning: --curl not supported in this build"?

> I wasn't sure whether to drop the define or not and figure it might be worth 
> keeping in case in change in the future for some reason.
> I don't mind dropping it and hardcofing the default to 1

I did not intend to suggest that. Just to be clear, I am very unfamiliar
with most of the Git codebase. Please don't take anything I say as
advice. :) As a question about something you have or haven't already
thought about, sure. :)

Martin

Re: [PATCH] tests: don't give unportable ">" to "test" built-in, use -gt

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> Change an argument to test_line_count (which'll ultimately be turned
> into a "test" expression) to use "-gt" instead of ">" for an
> arithmetic test.
>
> This broken on e.g. OpenBSD as of v2.13.0 with my commit
> ac3f5a3468 ("ref-filter: add --no-contains option to
> tag/branch/for-each-ref", 2017-03-24).
>
> Upstream just worked around it by patching git and didn't tell us

I'm tempted to do s/Upstream/Downstream/ while queuing, but please
tell me I am confused.

> about it, I discovered this when reading various Git packaging
> implementations: https://github.com/openbsd/ports/commit/7e48bf88a20
>
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
>
> David, it would be great to get a quick bug report to
> git@vger.kernel.org if you end up having to monkeypatch something
> we've done. We won't bite, promise :)
>
> As shown in that linked Github commit OpenBSD has another recent
> workaround in turning on DIR_HAS_BSD_GROUP_SEMANTICS and skipping a
> related test, maybe René can make more sense of that?
>
> There's more patches in their ports which indicate possible bugs of
> ours: https://github.com/openbsd/ports/tree/master/devel/git/patches/
>
>  t/t7004-tag.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index 0ef7b94394..0e2e57aa3d 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -1887,7 +1887,7 @@ EOF"
>   run_with_limited_stack git tag --contains HEAD >actual &&
>   test_cmp expect actual &&
>   run_with_limited_stack git tag --no-contains HEAD >actual &&
> - test_line_count ">" 10 actual
> + test_line_count "-gt" 10 actual
>  '
>  
>  test_expect_success '--format should list tags as per format given' '

Re: [git-for-windows] [ANNOUNCE] Git for Windows 2.14.0

[Johannes Sixt]

Am 07.08.2017 um 12:02 schrieb Johannes Schindelin:

On Sun, 6 Aug 2017, Johannes Sixt wrote:

Am 06.08.2017 um 01:00 schrieb Johannes Schindelin:

* Comes with [BusyBox v1.28.0pre.15857.9480dca7c](https://github.com/
  git-for-windows/busybox-w32/commit/9480dca7c].


What is the implication of this addition? I guess it is not just for the
fun of it. Does it mean that all POSIX command line tools invoked by Git
including a POSIX shell are now routed through busybox instead of the
MSYS2 variant?


As I wrote a little later:

* Git for Windows releases now also include an experimental [BusyBox-based
   
MinGit](https://github.com/git-for-windows/git/wiki/MinGit#experimental-busybox-based-mingit).


Thanks for the clue. It's an interesting concept. I would be interested 
in replacing my old MSYS environment by BusyBox. At best, it would be 
just a matter of replacing sh.exe.


-- Hannes

Re: [PATCH for NEXT v3 2/2] sub-process: refactor handshake to common function

[Jonathan Tan]

On Sun, 6 Aug 2017 21:58:24 +0200
Lars Schneider  wrote:

> > +   struct cmd2process *entry = (struct cmd2process *)subprocess;
> > +   return subprocess_handshake(subprocess, "git-filter", versions, NULL,
> > +   capabilities,
> > +   >supported_capabilities);
> 
> Wouldn't it make sense to add `supported_capabilities` to `struct 
> subprocess_entry` ?

The members of "struct subprocess_entry" are not supposed to be accessed
directly, according to the documentation. If we relaxed that, then we
could do this, but before that I think it's better to let the caller
handle it.

> > +
> > +static int handshake_version(struct child_process *process,
> > +const char *welcome_prefix, int *versions,
> 
> Maybe it would be less ambiguous if we call it `supported_versions` ? 

I thought of that, but I think "supported_versions" is actually more
ambiguous, since we don't know if these are versions supported by the
server or client or both.

> > +int *chosen_version)
> > +{
> > +   int version_scratch;
> > +   int i;
> > +   char *line;
> > +   const char *p;
> > +
> > +   if (!chosen_version)
> > +   chosen_version = _scratch;
> 
> I am not an C expert but wouldn't 'version_scratch' go out of scope as soon
> as the function returns? Why don't you error here right away?

It does, but so does chosen_version. This is meant to allow the caller
to pass NULL to this function.

> > +   if (packet_write_fmt_gently(process->in, "%s-client\n",
> > +   welcome_prefix))
> > +   return error("Could not write client identification");
> 
> Nit: Would it make sense to rename `welcome_prefix` to `client_id`?
> Alternatively, could we rename the error messages to "welcome prefix"?

I was retaining the existing terminology, but your suggestions seem
reasonable. This might be best done in another patch once this series
lands in master, though.

> > +   for (i = 0; versions[i]; i++) {
> > +   if (packet_write_fmt_gently(process->in, "version=%d\n",
> > +   versions[i]))
> > +   return error("Could not write requested version");
> 
> Maybe: "Could not write supported versions"?

Same as above - "supported" is ambiguous.

> > +   }
> > +   if (packet_flush_gently(process->in))
> > +   return error("Could not write flush packet");
> 
> I feel this error is too generic.
> Maybe: "Could not finish writing supported versions"?

That's reasonable. This is a rare error, though, and if it does occur, I
think this message is more informative. But I'm OK either way.

> > +
> > +   if (!(line = packet_read_line(process->out, NULL)) ||
> > +   !skip_prefix(line, welcome_prefix, ) ||
> > +   strcmp(p, "-server"))
> > +   return error("Unexpected line '%s', expected %s-server",
> > +line ? line : "", welcome_prefix);
> > +   if (!(line = packet_read_line(process->out, NULL)) ||
> > +   !skip_prefix(line, "version=", ) ||
> > +   strtol_i(p, 10, chosen_version))
> 
> Maybe `strlen("version=")` would be more clear than 10?

The 10 here is the base, not the length. If there's a better way to
convert strings to integers, let me know.

> > +   return error("Unexpected line '%s', expected version",
> 
> Maybe "... expected version number" ?

I'm fine either way.

> > +static int handshake_capabilities(struct child_process *process,
> > + struct subprocess_capability *capabilities,
> > + unsigned int *supported_capabilities)
> 
> I feel the naming could be misleading. I think ...
> `capabilities` is really `supported_capabilities` 
> and 
> `supported_capabilities` is really `negiotated_capabilties` or 
> `agreed_capabilites`

These "supported capabilities" are those supported by both the client
(Git) and the server (the process Git is invoking). I think it's better
to use this term for the intersection of capabilities, rather than
exclusively for the client or server.

> > +   for (i = 0; capabilities[i].name; i++) {
> > +   if (packet_write_fmt_gently(process->in, "capability=%s\n",
> > +   capabilities[i].name))
> > +   return error("Could not write requested capability");
> 
> I think this should be "Could not write supported capability", no?

Same comment as above.

> > +   }
> > +   if (packet_flush_gently(process->in))
> > +   return error("Could not write flush packet");
> 
> Maybe " "Could not finish writing supported capability" ?

Same comment as the one about writing flush packets above.

> > +   while ((line = packet_read_line(process->out, NULL))) {
> > +   const char *p;
> > +   if (!skip_prefix(line, "capability=", ))
> > +   continue;
> 
> Shouldn't we write an error in this case?

I'm preserving 

Re: [PATCH 1/4] imap-send: add wrapper to get server credentials if needed

[Martin Ågren]

On 7 August 2017 at 19:04, Nicolas Morey-Chaisemartin
 wrote:
>
>
> Le 07/08/2017 à 18:30, Martin Ågren a écrit :
>> On 7 August 2017 at 16:03, Nicolas Morey-Chaisemartin
>>  wrote:
>>> Signed-off-by: Nicolas Morey-Chaisemartin 
>>> ---
>>>  imap-send.c | 38 --
>>>  1 file changed, 24 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/imap-send.c b/imap-send.c
>>> index b2d0b849b..38b3c817e 100644
>>> --- a/imap-send.c
>>> +++ b/imap-send.c
>>> @@ -926,6 +926,29 @@ static int auth_cram_md5(struct imap_store *ctx, 
>>> struct imap_cmd *cmd, const cha
>>> return 0;
>>>  }
>>>
>>> +static void server_fill_credential(struct imap_server_conf *srvc)
>>> +{
>>> +   struct credential cred = CREDENTIAL_INIT;
>>> +
>>> +   if (srvc->user && srvc->pass)
>>> +   return;
>>> +
>>> +   cred.protocol = xstrdup(srvc->use_ssl ? "imaps" : "imap");
>>> +   cred.host = xstrdup(srvc->host);
>>> +
>>> +   cred.username = xstrdup_or_null(srvc->user);
>>> +   cred.password = xstrdup_or_null(srvc->pass);
>>> +
>>> +   credential_fill();
>>> +
>>> +   if (!srvc->user)
>>> +   srvc->user = xstrdup(cred.username);
>>> +   if (!srvc->pass)
>>> +   srvc->pass = xstrdup(cred.password);
>>> +
>>> +   credential_clear();
>>> +}
>>> +
>>>  static struct imap_store *imap_open_store(struct imap_server_conf *srvc, 
>>> char *folder)
>>>  {
>>> struct credential cred = CREDENTIAL_INIT;
>>> @@ -1078,20 +1101,7 @@ static struct imap_store *imap_open_store(struct 
>>> imap_server_conf *srvc, char *f
>>> }
>>>  #endif
>>> imap_info("Logging in...\n");
>>> -   if (!srvc->user || !srvc->pass) {
>>> -   cred.protocol = xstrdup(srvc->use_ssl ? "imaps" : 
>>> "imap");
>>> -   cred.host = xstrdup(srvc->host);
>>> -
>>> -   cred.username = xstrdup_or_null(srvc->user);
>>> -   cred.password = xstrdup_or_null(srvc->pass);
>>> -
>>> -   credential_fill();
>>> -
>>> -   if (!srvc->user)
>>> -   srvc->user = xstrdup(cred.username);
>>> -   if (!srvc->pass)
>>> -   srvc->pass = xstrdup(cred.password);
>>> -   }
>>> +   server_fill_credential(srvc);
>>>
>>> if (srvc->auth_method) {
>>> struct imap_cmd_cb cb;
>> "cred.username" is checked further down, but now it will always be NULL,
>> no?
>
> You're right I missed this.
> Not sure if this is needed though.
> From what I understand this means the username/password are store for the 
> next access to credential. but in the current state, there is only one.
> Maybe the credential_approved can be dropped ?

I'm no credentials-expert, but api-credentials.txt says this:

"Credential helpers are programs executed by Git to fetch or save
credentials from and to long-term storage (where "long-term" is simply
longer than a single Git process; e.g., credentials may be stored
in-memory for a few minutes, or indefinitely on disk)."

So the calls to approve/reject probably do matter in some scenarios.

The current code is a bit non-obvious as we just discovered since it
duplicates the strings (for good reasons, I believe) and then still
refers to the originals (also for good reasons, I believe). I suppose
your new function could be called like

server_fill_credential(, srvc);

That should limit the impact of the change, but I'm not sure it's a
brilliant interface. Just my 2c.

Martin

Re: [PATCH] Drop some dashes from built-in invocations in scripts

[Junio C Hamano]

Junio C Hamano  writes:

> Earlier there was a more ambitious proposal to remove all "git-foo"
> even from $GIT_EXEC_PATH for built-in commands, but that plan was
> scuttled [*1*].
>
> The changes in your patch still are good changes to make sure people
> who copy & paste code would see fewer instances of "git-foo", but
> "will still work even if I break my installation of Git by removing
> them from the filesystem" is not the project's goal.  
>
> IIUC, you will need "$GIT_EXEC_PATH/git-checkout" on the filesystem
> if you want your "git co" alias to work, as we spawn built-in as a
> dashed external.

Just to avoid possible confusion, the above is not to say "once it
is decided, you are not allowed to bring fresh arguments to the
discussion".  As Peff said [*2*] in that old discussion thread, the
circumstances may have changed over 9 years, and it may benefit to
revisit some old decisions.

So in that sense, I do not mind somebody makes a fresh proposal,
which would probably be similar to what I did back then in [*3*],
which is at the beginning of that old thread.  But I do not think I
would be doing so myself, and I suspect that I would not be very
supportive for such a proposal, because my gut feeling is that the
upside is not big enough compared to downsides.

The obvious upside is that you do not have to have many built-in
commands on the filesystem, either as a hardlink, a copy, or a
symbolic link.  But we will be breaking people's scripts by breaking
the 11-year old promise that we will keep their "git-foo" working as
long as they prepend $GIT_EXEC_PATH to their $PATH we we did so.
Another downside is that we now will expose which subcommands are
built-in and which are not, which is unnecessarily implementation
detail we'd want end-users to rely on.

The "'git co' may stop working" I mentioned in my previous message
is not counted as a downside---if the upside is large enough, I
think that "we respawn git-foo as dashed built-in when running an
alias that expands to 'foo'" can be fixed to respawn "git foo"
instead of "git-foo".  But there may be other downside that we may
not be able to fix, and I suspect that "we'd be breaking the 11-year
old promise" is something we would not be able to fix.  That is why
I doubt that I would be advocating the removal of "git-foo" from the
filesystem myself.


[References]

*1* 
https://public-inbox.org/git/alpine.lfd.1.10.0808261114070.3...@nehalem.linux-foundation.org/

*2* https://public-inbox.org/git/20080826145719.gb5...@coredump.intra.peff.net/

*3* https://public-inbox.org/git/7vprnzt7d5@gitster.siamese.dyndns.org/

Re: [PATCH 1/4] imap-send: add wrapper to get server credentials if needed

[Nicolas Morey-Chaisemartin]

Le 07/08/2017 à 18:30, Martin Ågren a écrit :
> On 7 August 2017 at 16:03, Nicolas Morey-Chaisemartin
>  wrote:
>> Signed-off-by: Nicolas Morey-Chaisemartin 
>> ---
>>  imap-send.c | 38 --
>>  1 file changed, 24 insertions(+), 14 deletions(-)
>>
>> diff --git a/imap-send.c b/imap-send.c
>> index b2d0b849b..38b3c817e 100644
>> --- a/imap-send.c
>> +++ b/imap-send.c
>> @@ -926,6 +926,29 @@ static int auth_cram_md5(struct imap_store *ctx, struct 
>> imap_cmd *cmd, const cha
>> return 0;
>>  }
>>
>> +static void server_fill_credential(struct imap_server_conf *srvc)
>> +{
>> +   struct credential cred = CREDENTIAL_INIT;
>> +
>> +   if (srvc->user && srvc->pass)
>> +   return;
>> +
>> +   cred.protocol = xstrdup(srvc->use_ssl ? "imaps" : "imap");
>> +   cred.host = xstrdup(srvc->host);
>> +
>> +   cred.username = xstrdup_or_null(srvc->user);
>> +   cred.password = xstrdup_or_null(srvc->pass);
>> +
>> +   credential_fill();
>> +
>> +   if (!srvc->user)
>> +   srvc->user = xstrdup(cred.username);
>> +   if (!srvc->pass)
>> +   srvc->pass = xstrdup(cred.password);
>> +
>> +   credential_clear();
>> +}
>> +
>>  static struct imap_store *imap_open_store(struct imap_server_conf *srvc, 
>> char *folder)
>>  {
>> struct credential cred = CREDENTIAL_INIT;
>> @@ -1078,20 +1101,7 @@ static struct imap_store *imap_open_store(struct 
>> imap_server_conf *srvc, char *f
>> }
>>  #endif
>> imap_info("Logging in...\n");
>> -   if (!srvc->user || !srvc->pass) {
>> -   cred.protocol = xstrdup(srvc->use_ssl ? "imaps" : 
>> "imap");
>> -   cred.host = xstrdup(srvc->host);
>> -
>> -   cred.username = xstrdup_or_null(srvc->user);
>> -   cred.password = xstrdup_or_null(srvc->pass);
>> -
>> -   credential_fill();
>> -
>> -   if (!srvc->user)
>> -   srvc->user = xstrdup(cred.username);
>> -   if (!srvc->pass)
>> -   srvc->pass = xstrdup(cred.password);
>> -   }
>> +   server_fill_credential(srvc);
>>
>> if (srvc->auth_method) {
>> struct imap_cmd_cb cb;
> "cred.username" is checked further down, but now it will always be NULL,
> no?

You're right I missed this.
Not sure if this is needed though.
>From what I understand this means the username/password are store for the next 
>access to credential. but in the current state, there is only one.
Maybe the credential_approved can be dropped ?

Nicolas

Re: [PATCH 4/4] imap-send: use curl by default

[Nicolas Morey-Chaisemartin]

Le 07/08/2017 à 18:37, Martin Ågren a écrit :
> On 7 August 2017 at 16:04, Nicolas Morey-Chaisemartin
>  wrote:
>> Signed-off-by: Nicolas Morey-Chaisemartin 
>> ---
>>  imap-send.c | 6 --
>>  1 file changed, 6 deletions(-)
>>
>> diff --git a/imap-send.c b/imap-send.c
>> index 90b8683ed..4ebc16437 100644
>> --- a/imap-send.c
>> +++ b/imap-send.c
>> @@ -35,13 +35,7 @@ typedef void *SSL;
>>  #include "http.h"
>>  #endif
>>
>> -#if defined(USE_CURL_FOR_IMAP_SEND) && defined(NO_OPENSSL)
>> -/* only available option */
>>  #define USE_CURL_DEFAULT 1
>> -#else
>> -/* strictly opt in */
>> -#define USE_CURL_DEFAULT 0
>> -#endif
>>
>>  static int verbosity;
>>  static int use_curl = USE_CURL_DEFAULT;
> So this is now basically "static int use_curl = 1;".
>
> Do we need a compile-time escape-hatch in case someone really needs
> to avoid curl, e.g., because they have a too old version? I suppose
> there is a conceptual difference between the "default", i.e., the value
> of USE_CURL_DEFAULT that is assigned to "use_curl", and the "default
> default", i.e., the value that is normally assigned to USE_CURL_DEFAULT.
>
> Martin

The curl code depends on USE_CURL_FOR_IMAP_SEND so even with use_curl == 1, it 
won't be an issue for people without curl (or old one).
I wasn't sure whether to drop the define or not and figure it might be worth 
keeping in case in change in the future for some reason.
I don't mind dropping it and hardcofing the default to 1

Also on a side note, I have a case where authentication works with --no-curl 
but fails withs --curl. Still trying to figure out why.

Nicolas

Re: upgarding GIT

[Todd Zullinger]

Ævar Arnfjörð Bjarmason wrote:

On Mon, Aug 07 2017, James Wells jotted:
I am fairly new to git, however I have a challenge of upgrading git 
from 2.0.4 to 2.4.12 and my initial 2.0.4 install was done via TAR 
BALL on my server.


I have a centos server running git and Atlassian STASH and my 
challenge is for me to upgrade the STASH application I need to move 
to a newer version of git.


Which release of CentOS are you using James?  And what git version is 
required for the Atlassian Stash (which is now called Bitbucket) 
release you're trying to use?  IIRC, they support as far back as git 
1.8?


You're going to want to install git via RPM/yum. CentOS already has a 
package for it.


Indeed.  (But I'm biased because I would never, ever install software 
via 'sudo make install' on anything other than a throw-away test 
instance.)


The one problem folks run into on CentOS/RHEL is that the versions may 
be somewhat old.  CentOS/RHEL 6 ships with git 1.7.1, for instance. 
CentOS/RHEL 7 is only a little newer, with git 1.8.3.  There are 
"software collections" which provide git 1.9¹ and 2.9², but personally 
I've never liked using software collections for software that I need 
to integrate with other tools.


For users of CentOS/RHEL who want to run the current git release in a 
packaged form, the Fedora git package maintainers take care to ensure 
that the Fedora packages can be built for the current supported 
releases of CentOS/RHEL (6 & 7 at the moment).  Grabbing the current 
code and/or srpm from Fedora should (almost³) always build cleanly 
using the mock build tool for CentOS/RHEL.


I also try to keep a semi-official COPR repo up to date, here:

   https://copr.fedorainfracloud.org/coprs/g/git-maint/git/

(Even as the primary maintainer of that repo, I'd still suggest that 
it's wise to either mirror it locally or rebuild the srpm's in your 
own infrastructure, to ensure that if the copr service is ever down 
you can reinstall important systems.)


¹ https://www.softwarecollections.org/en/scls/rhscl/git19/
² https://www.softwarecollections.org/en/scls/rhscl/rh-git29/
³ Right now, there's a slight issue building the current git for 
 CentOS 7 because RHEL 7.4 moved the pcre2 package from EPEL into 
 RHEL and CentOS 7.4 is not yet built.  The Fedora packages are 
 built against pcre2 now (thanks Ævar ;).  So for a few weeks it 
 won't be possible to build them for CentOS 7 without a minor change.


--
Todd
~~
Ocean, n. A body of water occupying about two-thirds of a world made
for man -- who has no gills.
   -- Ambrose Bierce, "The Devil's Dictionary"

Re: [PATCH 3/4] imap_send: setup_curl: use server_conf parameter instead of the global variable

[Nicolas Morey-Chaisemartin]

Le 07/08/2017 à 18:34, Martin Ågren a écrit :
> On 7 August 2017 at 16:04, Nicolas Morey-Chaisemartin
>  wrote:
>> Signed-off-by: Nicolas Morey-Chaisemartin 
>> ---
>>  imap-send.c | 24 
>>  1 file changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/imap-send.c b/imap-send.c
>> index 682a06551..90b8683ed 100644
>> --- a/imap-send.c
>> +++ b/imap-send.c
>> @@ -1415,37 +1415,37 @@ static CURL *setup_curl(struct imap_server_conf 
>> *srvc)
>> if (!curl)
>> die("curl_easy_init failed");
>>
>> -   server_fill_credential();
>> -   curl_easy_setopt(curl, CURLOPT_USERNAME, server.user);
>> -   curl_easy_setopt(curl, CURLOPT_PASSWORD, server.pass);
>> +   server_fill_credential(srvc);
>> +   curl_easy_setopt(curl, CURLOPT_USERNAME, srvc->user);
>> +   curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);
> Here you change the server_fill_credential-call that you just added.
> Maybe do this patch earlier, perhaps even as patch 1?
>
> I'm snipping lots of s/server/srvc/-changes... There's a less noisy
> way of addressing the fact that srvc is unused: dropping it. I'm not
> saying that's a good idea, but it could be considered, then explained
> why this approach is better. There are some other functions which
> access "server" directly, and some which take (and use!) a "srvc".
> Maybe make the whole file consistent?
>
> Martin
That's why I applied it after #2. I was not sure if this one made sense or not. 
And it  can be dropped with the rest of the series still applying.
I don't know what is the right approach here. Someone with more knowledge of 
why there is a mix of global variable and local can maybe help ?

Nicolas

Re: [PATCH 4/4] imap-send: use curl by default

[Martin Ågren]

On 7 August 2017 at 16:04, Nicolas Morey-Chaisemartin
 wrote:
> Signed-off-by: Nicolas Morey-Chaisemartin 
> ---
>  imap-send.c | 6 --
>  1 file changed, 6 deletions(-)
>
> diff --git a/imap-send.c b/imap-send.c
> index 90b8683ed..4ebc16437 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -35,13 +35,7 @@ typedef void *SSL;
>  #include "http.h"
>  #endif
>
> -#if defined(USE_CURL_FOR_IMAP_SEND) && defined(NO_OPENSSL)
> -/* only available option */
>  #define USE_CURL_DEFAULT 1
> -#else
> -/* strictly opt in */
> -#define USE_CURL_DEFAULT 0
> -#endif
>
>  static int verbosity;
>  static int use_curl = USE_CURL_DEFAULT;

So this is now basically "static int use_curl = 1;".

Do we need a compile-time escape-hatch in case someone really needs
to avoid curl, e.g., because they have a too old version? I suppose
there is a conceptual difference between the "default", i.e., the value
of USE_CURL_DEFAULT that is assigned to "use_curl", and the "default
default", i.e., the value that is normally assigned to USE_CURL_DEFAULT.

Martin

Re: [PATCH] Drop some dashes from built-in invocations in scripts

[Junio C Hamano]

Michael Forney  writes:

> This way, they still work even if the built-in symlinks aren't
> installed.
>
> Signed-off-by: Michael Forney 
> ---
> It looks like there was an effort to do this a number of years ago (through
> `make remove-dashes`). These are just a few I noticed were still left in the
> .sh scripts.

Our goal was *not* to have *no* "git-foo" on the filesystem,
though.  It happened in v1.6.0 timeframe and it was about removing
"git-foo" from end-user's $PATH.

Earlier there was a more ambitious proposal to remove all "git-foo"
even from $GIT_EXEC_PATH for built-in commands, but that plan was
scuttled [*1*].

The changes in your patch still are good changes to make sure people
who copy & paste code would see fewer instances of "git-foo", but
"will still work even if I break my installation of Git by removing
them from the filesystem" is not the project's goal.  

IIUC, you will need "$GIT_EXEC_PATH/git-checkout" on the filesystem
if you want your "git co" alias to work, as we spawn built-in as a
dashed external.


[Reference]

*1* 
https://public-inbox.org/git/alpine.lfd.1.10.0808261114070.3...@nehalem.linux-foundation.org/

Re: [PATCH 3/4] imap_send: setup_curl: use server_conf parameter instead of the global variable

[Martin Ågren]

On 7 August 2017 at 16:04, Nicolas Morey-Chaisemartin
 wrote:
> Signed-off-by: Nicolas Morey-Chaisemartin 
> ---
>  imap-send.c | 24 
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/imap-send.c b/imap-send.c
> index 682a06551..90b8683ed 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -1415,37 +1415,37 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
> if (!curl)
> die("curl_easy_init failed");
>
> -   server_fill_credential();
> -   curl_easy_setopt(curl, CURLOPT_USERNAME, server.user);
> -   curl_easy_setopt(curl, CURLOPT_PASSWORD, server.pass);
> +   server_fill_credential(srvc);
> +   curl_easy_setopt(curl, CURLOPT_USERNAME, srvc->user);
> +   curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);

Here you change the server_fill_credential-call that you just added.
Maybe do this patch earlier, perhaps even as patch 1?

I'm snipping lots of s/server/srvc/-changes... There's a less noisy
way of addressing the fact that srvc is unused: dropping it. I'm not
saying that's a good idea, but it could be considered, then explained
why this approach is better. There are some other functions which
access "server" directly, and some which take (and use!) a "srvc".
Maybe make the whole file consistent?

Martin

Re: [PATCH 1/4] imap-send: add wrapper to get server credentials if needed

[Martin Ågren]

On 7 August 2017 at 16:03, Nicolas Morey-Chaisemartin
 wrote:
> Signed-off-by: Nicolas Morey-Chaisemartin 
> ---
>  imap-send.c | 38 --
>  1 file changed, 24 insertions(+), 14 deletions(-)
>
> diff --git a/imap-send.c b/imap-send.c
> index b2d0b849b..38b3c817e 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -926,6 +926,29 @@ static int auth_cram_md5(struct imap_store *ctx, struct 
> imap_cmd *cmd, const cha
> return 0;
>  }
>
> +static void server_fill_credential(struct imap_server_conf *srvc)
> +{
> +   struct credential cred = CREDENTIAL_INIT;
> +
> +   if (srvc->user && srvc->pass)
> +   return;
> +
> +   cred.protocol = xstrdup(srvc->use_ssl ? "imaps" : "imap");
> +   cred.host = xstrdup(srvc->host);
> +
> +   cred.username = xstrdup_or_null(srvc->user);
> +   cred.password = xstrdup_or_null(srvc->pass);
> +
> +   credential_fill();
> +
> +   if (!srvc->user)
> +   srvc->user = xstrdup(cred.username);
> +   if (!srvc->pass)
> +   srvc->pass = xstrdup(cred.password);
> +
> +   credential_clear();
> +}
> +
>  static struct imap_store *imap_open_store(struct imap_server_conf *srvc, 
> char *folder)
>  {
> struct credential cred = CREDENTIAL_INIT;
> @@ -1078,20 +1101,7 @@ static struct imap_store *imap_open_store(struct 
> imap_server_conf *srvc, char *f
> }
>  #endif
> imap_info("Logging in...\n");
> -   if (!srvc->user || !srvc->pass) {
> -   cred.protocol = xstrdup(srvc->use_ssl ? "imaps" : 
> "imap");
> -   cred.host = xstrdup(srvc->host);
> -
> -   cred.username = xstrdup_or_null(srvc->user);
> -   cred.password = xstrdup_or_null(srvc->pass);
> -
> -   credential_fill();
> -
> -   if (!srvc->user)
> -   srvc->user = xstrdup(cred.username);
> -   if (!srvc->pass)
> -   srvc->pass = xstrdup(cred.password);
> -   }
> +   server_fill_credential(srvc);
>
> if (srvc->auth_method) {
> struct imap_cmd_cb cb;

"cred.username" is checked further down, but now it will always be NULL,
no?

RE: reftable [v5]: new ref storage format

[David Turner]

> -Original Message-
> From: Shawn Pearce [mailto:spea...@spearce.org]
> In git-core, I'm worried about the caveats related to locking. Git tries to 
> work
> nicely on NFS, and it seems LMDB wouldn't. Git also runs fine on a read-only
> filesystem, and LMDB gets a little weird about that. Finally, Git doesn't have
> nearly the risks LMDB has about a crashed reader or writer locking out future
> operations until the locks have been resolved. This is especially true with 
> shared
> user repositories, where another user might setup and own the semaphore.

FWIW, git has problems with stale lock file in the event of a crash 
(refs/foo.lock 
might still exist, and git does nothing to clean it up).

In my testing (which involved a *lot* of crashing), I never once had to clean 
up a
stale LMDB lock.  That said, I didn't test on a RO filesystem.

[PATCH 1/4] imap-send: add wrapper to get server credentials if needed

[Nicolas Morey-Chaisemartin]

Signed-off-by: Nicolas Morey-Chaisemartin 
---
 imap-send.c | 38 --
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index b2d0b849b..38b3c817e 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -926,6 +926,29 @@ static int auth_cram_md5(struct imap_store *ctx, struct 
imap_cmd *cmd, const cha
return 0;
 }
 
+static void server_fill_credential(struct imap_server_conf *srvc)
+{
+   struct credential cred = CREDENTIAL_INIT;
+
+   if (srvc->user && srvc->pass)
+   return;
+
+   cred.protocol = xstrdup(srvc->use_ssl ? "imaps" : "imap");
+   cred.host = xstrdup(srvc->host);
+
+   cred.username = xstrdup_or_null(srvc->user);
+   cred.password = xstrdup_or_null(srvc->pass);
+
+   credential_fill();
+
+   if (!srvc->user)
+   srvc->user = xstrdup(cred.username);
+   if (!srvc->pass)
+   srvc->pass = xstrdup(cred.password);
+
+   credential_clear();
+}
+
 static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char 
*folder)
 {
struct credential cred = CREDENTIAL_INIT;
@@ -1078,20 +1101,7 @@ static struct imap_store *imap_open_store(struct 
imap_server_conf *srvc, char *f
}
 #endif
imap_info("Logging in...\n");
-   if (!srvc->user || !srvc->pass) {
-   cred.protocol = xstrdup(srvc->use_ssl ? "imaps" : 
"imap");
-   cred.host = xstrdup(srvc->host);
-
-   cred.username = xstrdup_or_null(srvc->user);
-   cred.password = xstrdup_or_null(srvc->pass);
-
-   credential_fill();
-
-   if (!srvc->user)
-   srvc->user = xstrdup(cred.username);
-   if (!srvc->pass)
-   srvc->pass = xstrdup(cred.password);
-   }
+   server_fill_credential(srvc);
 
if (srvc->auth_method) {
struct imap_cmd_cb cb;

Re: Change in output as a result of patch

[Kaartic Sivaraam]

On Mon, 2017-07-24 at 14:25 -0700, Junio C Hamano wrote:
> I suspect that with a moderately-sized refactoring around
> validate_new_branchname() function, this should be doable.  Instead
> of passing two "int" parameters force and attr_only, make them into
> a single "unsigned flag"
I guess it's not possible to merge the two parameters into one as the
following code path shouldn't be taken when 'attr_only' is set,

if (!attr_only) {
const char *head;
struct object_id oid;

head = resolve_ref_unsafe("HEAD", 0, oid.hash, NULL);
if (!is_bare_repository() && head && !strcmp(head, ref->buf))
die(_("Cannot force update the current branch."));
}

and I guess this means the 'attr_only' can't merged with 'force'.

Further, I saw this in 'branch.h'

>  NEEDSWORK: This needs to be split into two separate functions in the
>  longer run for sanity.

Any ways in which I could help with this?

-- 
Kaartic

Re: reftable [v5]: new ref storage format

[Shawn Pearce]

On Sun, Aug 6, 2017 at 4:37 PM, Ben Alex  wrote:
> Just on the LmdbJava specific pieces:
>
> On Mon, Aug 7, 2017 at 8:56 AM, Shawn Pearce  wrote:
>>
>> Looks pretty complete. Its a Java wrapper around the C implementation
>> of LMDB, which may be sufficient for reference storage. Keys are
>> limited to 511 bytes, so insanely long reference names would have to
>> be rejected. Reftable allows reference names up to the file's
>> `page_size`, minus overhead (~15 bytes) and value (20 bytes).
>
>
> For clarification LmdbJava code doesn't enforce a particular key size limit.
> For puts the caller nominates the size in the buffer they present for
> storage, and for get-style operations (cursors etc) the LMDB database stores
> the key size and LmdbJava adjusts the Java-visible buffer accordingly.
>
> A 511 byte key limit is specified at compile time for the native LMDB
> library. For convenience the native library is compiled for 64-bit Windows,
> Linux and Mac OS and included in the LmdbJava JAR, and this compilation is
> performed using default values (including the 511 key limit) by the
> https://github.com/lmdbjava/native project. Users can specify a different
> native library to use (eg one packaged by their OS or separately compiled
> using an LmdbJava Native-like automatic build) with a larger key size if
> they wish.
>
> As such if JGit wanted to use a longer key size, it is possible to implement
> similar automatic builds and packaging into JGit.

I don't know if we need a larger key size. $DAY_JOB limits ref names
to ~200 bytes in a hook. I think GitHub does similar. But I'm worried
about the general masses who might be using our software and expect
ref names thus far to be as long as PATH_MAX on their system. Most
systems run PATH_MAX around 1024.

The limitation of needing native JARs, and having such a low compile
time constant, may be annoying to some.

>> A downside for JGit is getting these two open source projects cleared.
>> We would have to get approval from our sponsor (Eclipse Foundation) to
>> use both lmdbjava (Apache License) and LMDB (LMDB license).
>
>
> I can't speak for the other contributors, but I'm happy to review LmdbJava's
> license if this assisted. For example changing to the OpenLDAP License would
> seem a reasonable variation given users of LmdbJava already need to accept
> the OpenLDAP License to use it. Kristoffer, do you have thoughts on this?

Thanks for considering it, but please don't change your licensing just
because of JGit. Its unlikely we can use LMDB for a lot of technical
reasons.

>> Plus it
>> looks like lmdbjava still relies on local disk and isn't giving us a
>> way to patch in a virtual filesystem the way I need to at $DAY_JOB.
>
>
> LMDB's mdb_env_open requires a const char* path, so we can pass through any
> char array desired. But I think you'll find LMDB native can't map to a
> virtual file system implemented by JVM code (the LMDB caveats section has
> further local file system considerations).

Mostly at $DAY_JOB its because we can't virtualize the filesystem
calls the C library is doing.

In git-core, I'm worried about the caveats related to locking. Git
tries to work nicely on NFS, and it seems LMDB wouldn't. Git also runs
fine on a read-only filesystem, and LMDB gets a little weird about
that. Finally, Git doesn't have nearly the risks LMDB has about a
crashed reader or writer locking out future operations until the locks
have been resolved. This is especially true with shared user
repositories, where another user might setup and own the semaphore.

[PATCH 2/2 / RFC] branch: quote branch/ref names to improve readability

[Kaartic Sivaraam]

Signed-off-by: Kaartic Sivaraam 
---
 branch.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/branch.c b/branch.c
index ad5a2299b..a40721f3c 100644
--- a/branch.c
+++ b/branch.c
@@ -90,24 +90,24 @@ int install_branch_config(int flag, const char *local, 
const char *origin, const
if (shortname) {
if (origin)
printf_ln(rebasing ?
- _("Branch %s set up to track remote 
branch %s from %s by rebasing.") :
- _("Branch %s set up to track remote 
branch %s from %s."),
+ _("Branch '%s' set up to track remote 
branch '%s' from '%s' by rebasing.") :
+ _("Branch '%s' set up to track remote 
branch '%s' from '%s'."),
  local, shortname, origin);
else
printf_ln(rebasing ?
- _("Branch %s set up to track local 
branch %s by rebasing.") :
- _("Branch %s set up to track local 
branch %s."),
+ _("Branch '%s' set up to track local 
branch '%s' by rebasing.") :
+ _("Branch '%s' set up to track local 
branch '%s'."),
  local, shortname);
} else {
if (origin)
printf_ln(rebasing ?
- _("Branch %s set up to track remote 
ref %s by rebasing.") :
- _("Branch %s set up to track remote 
ref %s."),
+ _("Branch '%s' set up to track remote 
ref '%s' by rebasing.") :
+ _("Branch '%s' set up to track remote 
ref '%s'."),
  local, remote);
else
printf_ln(rebasing ?
- _("Branch %s set up to track local 
ref %s by rebasing.") :
- _("Branch %s set up to track local 
ref %s."),
+ _("Branch '%s' set up to track local 
ref '%s' by rebasing.") :
+ _("Branch '%s' set up to track local 
ref '%s'."),
  local, remote);
}
}
-- 
2.14.0.rc1.434.g6eded367a

[PATCH 1/2 / RFC] builtin/branch: remove the deprecated '--set-upstream' option

[Kaartic Sivaraam]

The '--set-upstream' option of branch was deprecated in,

b347d06bf branch: deprecate --set-upstream and show help if we detect
possible mistaken use (Thu, 30 Aug 2012 19:23:13 +0200)

It was deprecated for the reasons specified in the commit message of
the referenced commit.

Refactor 'branch' so that it doesn't accept '--set-upstream'.

Note that, 'git branch' still *accepts* '--set-upstream' as a consequence
of "unique prefix can be abbrievated in option names". '--set-upstream'
is a unique prefix of '--set-upstream-to' after '--set-upstream' has
been removed.

The before/after behaviour for a simple case follows,

$ git remote
origin

Before,

$ git branch
* master

$ git branch --set-upstream origin/master
The --set-upstream flag is deprecated and will be removed. Consider using 
--track or --set-upstream-to
Branch origin/master set up to track local branch master.

$ git branch
* master
  origin/master

After,

$ git branch
* master

$ git branch --set-upstream origin/master
Branch master set up to track remote branch master from origin.

$ git branch
* master

Note that the option used in the after sequence is still '--set-upstream'
though the behaviour is that of '--set-upstream-to'.

Signed-off-by: Kaartic Sivaraam 
---
 Documentation/git-branch.txt | 10 ++---
 builtin/branch.c | 24 -
 t/t3200-branch.sh| 50 ++--
 3 files changed, 4 insertions(+), 80 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 81bd0a7b7..23c47b850 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -14,7 +14,7 @@ SYNOPSIS
[(--merged | --no-merged) []]
[--contains [ 0 && argc <= 2) {
struct branch *branch = branch_get(argv[0]);
-   int branch_existed = 0, remote_tracking = 0;
-   struct strbuf buf = STRBUF_INIT;
 
if (!strcmp(argv[0], "HEAD"))
die(_("it does not make sense to create 'HEAD' 
manually"));
@@ -767,29 +763,9 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
if (filter.kind != FILTER_REFS_BRANCHES)
die(_("-a and -r options to 'git branch' do not make 
sense with a branch name"));
 
-   if (track == BRANCH_TRACK_OVERRIDE)
-   fprintf(stderr, _("The --set-upstream flag is 
deprecated and will be removed. Consider using --track or 
--set-upstream-to\n"));
-
-   strbuf_addf(, "refs/remotes/%s", branch->name);
-   remote_tracking = ref_exists(buf.buf);
-   strbuf_release();
-
-   branch_existed = ref_exists(branch->refname);

Can the '--set-upstream' option of branch be removed ?

[Kaartic Sivaraam]

I refactored builtin/branch.c to remove the '--set-upstream'
option,successfully. The corresponding patch follows. 

There's just one issue with the version of git that doesn't
have the '--set-upstream' option. It's described in the commit
log message of the following patch.

I guess it would be difficult to detect the removal of the option in
case it's used in scripts and might cause confusion to users?
Is it ok to proceed with the removal?

BTW, It's now clear  to me that removing '--set-upstream' has nothing
to do with merging the two parameter of 'validate_new_branchname'.

-- 
Kaartic

Re: [PATCH 2/2] doc: add another way to identify if a patch has been merged

[Kaartic Sivaraam]

On Wed, 2017-08-02 at 10:58 -0700, Stefan Beller wrote:
> That may be either a contributors problem (lacking time or
> motivation to go through a long document) or a problem with
> the community.
> 
I'm trying to avoid the former.

> I would not want to explain the same thing over and over again,
> but rather have a technical solution that explains the problem and
> solution once it is detected.
> 
> Coming up with a technical solution for each little quirk
> is not the hard part (e.g. grep for the sign off string, count lines of
> the commit message), but rather to put it in place. (How can I make
> sure that contributors run these small checker scripts?
> Currently I cannot.)
> 
I could see quite some alternatives for this.

1. scripts

I guess the kernel community use some scripts to check if the patch
has the required style.[ref 1][ref 2]. I guess we could do something
similar. Like writing a script that checks the log messages for the
required format (sign-off, area etc.) and giving users advice about
how to fix the issue. After a all script test pass we could give
some advice to the user about how the patch needs to be sent.

To identify the set of commit messages that need to be checked we
could make the script accept a single parameter that specifies the
base of the branch. I'm not sure if this part could be automated.

2. Hooks

warning: this might be a little over thought.

1. Code all the checks as 'hooks scripts' that aren't samples.
Possibly scripts related to 'commit-msg'.

2. Place them in a 'hooks' directory under a new directory, possibly
named 'hook-checks'.

3. Inform the new contributor to re-initialize his git.git with

$ git init --template=/path/to/git/hook-checks

4. Rebasing their commits with 'rewording' each

Of course, this relies on the fact that he wouldn't have enabled
hooks in their git.git. In which case he would have to merge the
scripts with his own scripts.

I'm not pretty sure if they're feasible or not.

-- 
Kaartic

Re: [PATCH 2/2] doc: add another way to identify if a patch has been merged

[Kaartic Sivaraam]

On Wed, 2017-08-02 at 09:01 -0700, Junio C Hamano wrote:
> Kaartic Sivaraam  writes:
> 
> > On Tue, 2017-08-01 at 10:46 -0700, Stefan Beller wrote:
> > > So maybe we want to cut a lot of workflow related commendatory from
> > > the SubmitingPatches and then encourage to read such man page?
> > > 
> > 
> > That's right. Maybe Documentation/SubmittingPatches needs a revamp to
> > be one-time contributor friendly? Maybe introducing a "gist" for people
> > who do not have the time to read the whole document, might be of help?
> 
> First of all, I do not think lack of one-time contributor is
> something we should consider to be a problem.  Supporting new
> contributors so that they will be involved more in the development
> process is a lot more important issue.
> 
First of all, I would like to clear a little mis-interpretation here.
Though I used the phrase 'one-time contributor', I didn't want a gist
that targets only *those* people. I was thinking, in general, about
people who would like to contribute but find the documentation
overwhelming (an example might be the thread pointed out by Stefan). In
which case, they would want to check if their patch meets the *basic
criterias* and send it to the community hoping it would be accepted
with at least 75% probability.

I'll send a patch that tries to make 'Documentation/SubmittingPatches'
less overwhelming without losing much of it's content, some time soon.

> I think the exchange Stefan cited was an example that we want to
> have more of.  The contributor is indicating that, even though the
> patch could be a drive-by patch by one-timer from whom we will never
> hear again, it is not--the contributor is willing to learn the way
> things are done here, and showing that it is worth _our_ time to
> explain the things so that the future patches will take less effort
> to accept on our side.
> 
I thought a *good* 'gist' would obviate that kind of effort. Let's see
if I could come up with something.

> Because we do not have a group of dedicated volunteers, it is done
> by more experienced people around here but that can be done only
> when they have time.  I view it as a more severe problem than any
> documentation.  An abbreviated version of the documentation to
> invite more new people means that we must be prepared to give more
> high-touch onboarding help to them.
> 
I think an abbrievated documentation whilst inviting new people
*should* obviate the onboarding help, saving everyone's time (win-win).

-- 
Kaartic

[PATCH 4/4] imap-send: use curl by default

[Nicolas Morey-Chaisemartin]

Signed-off-by: Nicolas Morey-Chaisemartin 
---
 imap-send.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 90b8683ed..4ebc16437 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -35,13 +35,7 @@ typedef void *SSL;
 #include "http.h"
 #endif
 
-#if defined(USE_CURL_FOR_IMAP_SEND) && defined(NO_OPENSSL)
-/* only available option */
 #define USE_CURL_DEFAULT 1
-#else
-/* strictly opt in */
-#define USE_CURL_DEFAULT 0
-#endif
 
 static int verbosity;
 static int use_curl = USE_CURL_DEFAULT;
-- 
2.14.0.rc1.16.g87fcec1e8