Re: [PATCH v1] convert: add support for 'encoding' attribute

[Johannes Sixt]

Am 12.12.2017 um 01:59 schrieb Junio C Hamano:

Stepping back a bit, what does this thing do you are introducing?
And what does the other thing do that J6t is using, that would get
confused with this new one?

What does the other one do?  "Declare that the contents of this path
is in this encoding"?  As opposed to the new one, which tells Git to
"run iconv from and to this encoding when checking out and checking
in"?

If so, any phrase that depends heavily on the word "encode" would
not help differenciating the two uses.  The phrase needs to be
something that contrasts the new one, which actively modifies things
(what is on the filesystem is not what is stored in the object
store), with the old one, which does not (passed as a declaration to
a viewer what encoding the contents already use and does not change
anything).


Well explained!


...  perhaps "smudge-encoding" would work (we declare that the
result of smudge operations are left in this encoding, so the
opposite operation "clean" will do the reverse---and we say this
without explicitly saying that the other end of the conversion is
always UTF-8)?  Or "checkout-encoding" (the same explanation; we do
not say the opposite operation "checkin/add" will do the reverse).


I would favor "checkout-encoding" over "smudge-encoding" only because 
"checkout" is better known than "smudge", I would think. I do not have 
better suggestions.


-- Hannes

Re: [PATCH v1] convert: add support for 'encoding' attribute

[Johannes Sixt]

Am 12.12.2017 um 00:42 schrieb Lars Schneider:

BTW: I am curios, can you share what encoding you use?
My main use case is UTF-16 and I was surprised that I haven't
found a single public repo on github.com with "encoding=utf-16"


Shift-JIS and CP1252. These are used for Windows resource files (*.rc).

-- Hannes

Dear Friend,

[Mr Akpala Maiky]

Dear Friend,

I am the head of Accounts and Audit Department of Bank of Africa,
Ouagadougou . I decided to contact you after a careful thought that
you may be capable of handling this business transaction which I
explained below;
In my department, I discovered an abandoned sum of $13.5m US dollars
(Thirteen million, five hundred thousand US dollars). In an account
that belongs to one of our foreign customer who died along with his
entire family in 2007 in a plane crash.

Since i got information about his death, The bank have been expecting
his next of kin to come over and claim his money because The fund
cannot be released unless somebody applies for it as next of kin or
relation to the deceased as indicated in our banking guidelines but
unfortunately I learnt that his supposed next of kin(his son and wife)
died alongside with him at the plane crash leaving nobody behind for
the claim .It is therefore upon this discovery that I now decided to
make this business proposal to you and release the money to you as the
next of kin (I want to present you as his business associate )to the
deceased for safety and subsequent disbursement since nobody is coming
for it and I don't want this money to go into the Bank treasury as
unclaimed Bill.

The Banking law and guideline here stipulates that if such money
remained Unclaimed after nine years, the money will be transferred
into the Bank treasury as unclaimed fund.. The request of foreigner as
next of kin in this business is occasioned by the fact that the
customer was a foreigner and a Burkina be cannot stand as next of kin
to a foreigner.

I agree that 35% of this money will be for you as foreign partner, in
respect to the provision of a foreign account, 10 % will be set aside
for expenses incurred during the business and 55% would be for me .
There after I will visit your country for disbursement according to
the percentages indicated. Therefore to enable the immediate transfer
of this fund to your account as arranged, you must apply first to the
bank as next of kin of the deceased customer.

Upon receipt of your reply, I will send to you by fax or email the
text of the application. I will not fail to bring to your notice that
this transaction is hitch free and that you should not entertain any
atom of fear as all required arrangements have been made for the
transfer..

I expect that you contact me immediately as soon as you receive this
letter, and send me your personal data including your scan copy of
your identification  for continuation of this transaction.Reach me
through akpalara...@gmail.com for urgent response.

Hoping to hear from you immediately.

Yours faithfully,
Mr Akpala Maiky
Accounts & Audit Department,
Bank of Africa.

[PATCH] diffcore: add a filter to find a specific blob

[Stefan Beller]

Sometimes users are given a hash of an object and they want to
identify it further (ex.: Use verify-pack to find the largest blobs,
but what are these? or [1])

One might be tempted to extend git-describe to also work with blobs,
such that `git describe ` gives a description as
':'.  This was implemented at [2]; as seen by the sheer
number of responses (>110), it turns out this is tricky to get right.
The hard part to get right is picking the correct 'commit-ish' as that
could be the commit that (re-)introduced the blob or the blob that
removed the blob; the blob could exist in different branches.

Junio hinted at a different approach of solving this problem, which this
patch implements. Teach the diff machinery another flag for restricting
the information to what is shown. For example:

  $ ./git log --oneline --find-object=v2.0.0:Makefile
  b2feb64309 Revert the whole "ask curl-config" topic for now
  47fbfded53 i18n: only extract comments marked with "TRANSLATORS:"

we observe that the Makefile as shipped with 2.0 was appeared in
v1.9.2-471-g47fbfded53 and in v2.0.0-rc1-5-gb2feb6430b.  The
reason why these commits both occur prior to v2.0.0 are evil
merges that are not found using this new mechanism.

[1] https://stackoverflow.com/questions/223678/which-commit-has-this-blob
[2] https://public-inbox.org/git/20171028004419.10139-1-sbel...@google.com/

Signed-off-by: Stefan Beller 

asdf

Signed-off-by: Stefan Beller 
---
 Documentation/diff-options.txt |  6 
 Makefile   |  1 +
 builtin/log.c  |  2 +-
 diff.c | 20 -
 diff.h |  3 ++
 diffcore-objfind.c | 42 ++
 diffcore.h |  1 +
 revision.c |  5 +++-
 t/t4064-diff-oidfind.sh| 68 ++
 9 files changed, 145 insertions(+), 3 deletions(-)
 create mode 100644 diffcore-objfind.c
 create mode 100755 t/t4064-diff-oidfind.sh

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index dd0dba5b1d..21d1776996 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -500,6 +500,12 @@ information.
 --pickaxe-regex::
Treat the  given to `-S` as an extended POSIX regular
expression to match.
+
+--find-object=::
+   Restrict the output such that one side of the diff
+   matches the given object id. The object can be a blob,
+   gitlink entry or tree (when `-t` is given).
+
 endif::git-format-patch[]
 
 -O::
diff --git a/Makefile b/Makefile
index ee9d5eb11e..c26596c30a 100644
--- a/Makefile
+++ b/Makefile
@@ -775,6 +775,7 @@ LIB_OBJS += date.o
 LIB_OBJS += decorate.o
 LIB_OBJS += diffcore-break.o
 LIB_OBJS += diffcore-delta.o
+LIB_OBJS += diffcore-objfind.o
 LIB_OBJS += diffcore-order.o
 LIB_OBJS += diffcore-pickaxe.o
 LIB_OBJS += diffcore-rename.o
diff --git a/builtin/log.c b/builtin/log.c
index 6c1fa896ad..08ea82d69f 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -181,7 +181,7 @@ static void cmd_log_init_finish(int argc, const char 
**argv, const char *prefix,
init_display_notes(>notes_opt);
 
if (rev->diffopt.pickaxe || rev->diffopt.filter ||
-   rev->diffopt.flags.follow_renames)
+   rev->diffopt.flags.follow_renames || rev->diffopt.objfind)
rev->always_show_header = 0;
 
if (source)
diff --git a/diff.c b/diff.c
index 0763e89263..e13b8229d3 100644
--- a/diff.c
+++ b/diff.c
@@ -4082,6 +4082,7 @@ void diff_setup(struct diff_options *options)
options->interhunkcontext = diff_interhunk_context_default;
options->ws_error_highlight = ws_error_highlight_default;
options->flags.rename_empty = 1;
+   options->objfind = NULL;
 
/* pathchange left =NULL by default */
options->change = diff_change;
@@ -4487,6 +4488,19 @@ static int parse_ws_error_highlight_opt(struct 
diff_options *opt, const char *ar
return 1;
 }
 
+static int parse_objfind_opt(struct diff_options *opt, const char *arg)
+{
+   struct object_id oid;
+
+   if (get_oid(arg, ))
+   return error("unable to resolve '%s'", arg);
+
+   if (!opt->objfind)
+   opt->objfind = xcalloc(1, sizeof(*opt->objfind));
+   oidset_insert(opt->objfind, );
+   return 1;
+}
+
 int diff_opt_parse(struct diff_options *options,
   const char **av, int ac, const char *prefix)
 {
@@ -4736,7 +4750,8 @@ int diff_opt_parse(struct diff_options *options,
else if ((argcount = short_opt('O', av, ))) {
options->orderfile = prefix_filename(prefix, optarg);
return argcount;
-   }
+   } else if (skip_prefix(arg, "--find-object=", ))
+   return parse_objfind_opt(options, arg);
else if ((argcount = parse_long_opt("diff-filter", av, ))) {
int offending = 

Re: Q: rational for $XDG_CONFIG_HOME/git/config to be "non global" or just a bug?

[Junio C Hamano]

Jonathan Nieder  writes:

> I think the documentation
>
>   ~/.gitconfig
>   User-specific configuration file. Also called "global"
>   configuration file.
>
> should be clarified --- e.g. it could say
>
>   $XDG_CONFIG_HOME/git/config
>   ~/.gitconfig
>   User-specific configuration files. Because options in
>   these files are not specific to any repository, thes
>   are sometimes called global configuration files.

Yeah, I think that makes sense.

> As for "git config --global", I think the best thing would be to split
> it into two options: something like "git config --user" and "git
> config --xdg-user".  That way, it is unambiguous which configuration
> file the user intends to inspect or modify.  When a user calls "git
> config --global" and both files exist, it could warn that the command
> is ambiguous.
>
> Thoughts?

I actually thought that the plan was "you either have this, or the
other one, never both at the same time" (and I think those who
pushed the XDG thing in to the system made us favor it over the
traditional one).  So as long as --global updates the one that
exists, and updates XDG one when both or neither do, I think we
should be OK.  And from that viewpoint, we definitely do not want
two kinds of --global to pretend as if we support use of both at the
same time.

Re: [PATCH v1] convert: add support for 'encoding' attribute

[Junio C Hamano]

Lars Schneider  writes:

> I contemplated:
>   - "enc" or "encode" because "eol" and "ident" use abbreviations, too
> (enc could be confused with encryption. plus, a user might ask
>  what is the difference between "enc" and "encoding" attribute :-)
>   - "wte", "wtenc", or "worktree-encoding" to emphasize that this is 
> the encoding used in the worktree 
> (I fear that users think that is git-worktree, the command, related)

In the context of Git, the word "worktree" does have a specific
meaning that is different from working tree.  

Stepping back a bit, what does this thing do you are introducing?
And what does the other thing do that J6t is using, that would get
confused with this new one?

What does the other one do?  "Declare that the contents of this path
is in this encoding"?  As opposed to the new one, which tells Git to
"run iconv from and to this encoding when checking out and checking
in"?

If so, any phrase that depends heavily on the word "encode" would
not help differenciating the two uses.  The phrase needs to be
something that contrasts the new one, which actively modifies things
(what is on the filesystem is not what is stored in the object
store), with the old one, which does not (passed as a declaration to
a viewer what encoding the contents already use and does not change
anything).

Do people who will use this feature familiar with the concept of
smudge/clean?  If you want to avoid "working-tree" (or "worktree",
which definitely you would want to avoid) because you fear confused
users, perhaps "smudge-encoding" would work (we declare that the
result of smudge operations are left in this encoding, so the
opposite operation "clean" will do the reverse---and we say this
without explicitly saying that the other end of the conversion is
always UTF-8)?  Or "checkout-encoding" (the same explanation; we do
not say the opposite operation "checkin/add" will do the reverse).

I personally do not think "working-tree-encoding" is too horrible,
but I do agree that some users may be confused.  So I dunno.

Re: [PATCH] clone: support 'clone --shared' from a worktree

[Brandon Williams]

On 12/11, Eric Sunshine wrote:
> On Mon, Dec 11, 2017 at 7:18 PM, Brandon Williams  wrote:
> > On 12/11, Eric Sunshine wrote:
> >>   struct strbuf alt = STRBUF_INIT;
> >> - strbuf_addf(, "%s/objects", src_repo);
> >> + get_common_dir(, src_repo);
> >> + strbuf_addstr(, "/objects");
> >
> > If you wanted to do this in one function call you could either use
> > 'strbuf_git_common_path()' or either 'strbuf_git_path()' or
> > 'strbuf_repo_git_path()' which will do the proper path adjustments when
> > working on a path which should be shared between worktrees (i.e. part of
> > the common git dir).
> 
> Thanks for the pointers, however, the above fix mirrors the fix made
> by 744e469755 (clone: allow --local from a linked checkout,
> 2015-09-28) to code immediately below it in the 'else' arm:
> 
> get_common_dir(, src_repo);
> get_common_dir(, dest_repo);
> strbuf_addstr(, "/objects");
> strbuf_addstr(, "/objects");
> 
> It would be poor form and confusing to use one of the mechanisms you
> suggest while leaving the 'else' arm untouched.
> 
> Re-working both arms of the 'if' to use one of the suggested functions
> would make a fine follow-on or preparatory patch, however, I'd rather
> not hold up this fix merely to re-roll for such a minor cleanup. (I
> also considered a follow-on patch to reduce the duplication between
> the two cases but decided against it, for the present, since such a
> patch would almost be noise without much gain.)

I didn't look close enough at what you were trying to fix, you're right
I think what you have here is good as the alternative would require a
lot more reworking I think (at least to change the above part too).

Either way though, I'm a little worried about what happens if you have
GIT_COMMON_DIR set because then both the src and dest repo would share a
common dir, I don't know if that is expected or not.  Maybe something
else to consider later.

> 
> By the way, is there any documentation explaining the differences
> between all these similar functions and when one should be used over
> the others?

I wish, I probably should have done a better job documenting it all in
path.h when I added the repo_* flavor of functions.  I'll add that to my
list of things to do though :)

-- 
Brandon Williams

Re: Q: rational for $XDG_CONFIG_HOME/git/config to be "non global" or just a bug?

[Yaroslav Halchenko]

On Mon, 11 Dec 2017, Jonathan Nieder wrote:
> > Example to show that TFM outlines precedence and --global correctly:

> > $> grep xdg .gitconfig .config/git/config
> > .gitconfig:xdg-and-user = user
> > .config/git/config: xdg = xdg
> > .config/git/config: xdg-and-user = xdg
> > $> git config user.xdg ; git config user.xdg-and-user
> > xdg
> > user

> I agree, this is confusing.

> Reverse engineering from source, I find that git reads the following
> files in sequence:

>   system:
>   /etc/gitconfig
>   global:
>   $XDG_CONFIG_HOME/git/config
>   $HOME/.gitconfig
>   repo:
>   $GIT_DIR/config
>   commandline:
>   options passed with -c or GIT_CONFIG_PARAMETERS

> These terms (system, global, repo, etc) are accessible in code as
> current_config_scope().  I don't think there's any user-visible effect
> to $XDG_CONFIG_HOME/git/config and $HOME/.gitconfig both being global
> --- it would probably be a good cleanup to rename the scope for one of
> them.

Well, we have got at least one user/contributor now who uses
$XDG_CONFIG_HOME/git/config in favor of ~/.gitconfig since it makes it
easier for modular user configuration.

> I think the documentation

>   ~/.gitconfig
>   User-specific configuration file. Also called "global"
>   configuration file.

> should be clarified --- e.g. it could say

>   $XDG_CONFIG_HOME/git/config
>   ~/.gitconfig
>   User-specific configuration files. Because options in
>   these files are not specific to any repository, thes
>   are sometimes called global configuration files.

> As for "git config --global", I think the best thing would be to split
> it into two options: something like "git config --user" and "git
> config --xdg-user".  That way, it is unambiguous which configuration
> file the user intends to inspect or modify.  When a user calls "git
> config --global" and both files exist, it could warn that the command
> is ambiguous.

why ambiguous?  as long as both are consistently called global, and the
overloading rules are clear for reading -- nothing ambigous.  The only
ambigous logic would be for writing.

> Thoughts?

Well -- my main functionality concern that ATM
$XDG_CONFIG_HOME/git/config is (as of 2.15.0) only --global for writing
but not for regular reading (as I demonstrated in the original email)

-- 
Yaroslav O. Halchenko
Center for Open Neuroscience http://centerforopenneuroscience.org
Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755
Phone: +1 (603) 646-9834   Fax: +1 (603) 646-1419
WWW:   http://www.linkedin.com/in/yarik

Re: [PATCH] clone: support 'clone --shared' from a worktree

[Eric Sunshine]

On Mon, Dec 11, 2017 at 7:18 PM, Brandon Williams  wrote:
> On 12/11, Eric Sunshine wrote:
>>   struct strbuf alt = STRBUF_INIT;
>> - strbuf_addf(, "%s/objects", src_repo);
>> + get_common_dir(, src_repo);
>> + strbuf_addstr(, "/objects");
>
> If you wanted to do this in one function call you could either use
> 'strbuf_git_common_path()' or either 'strbuf_git_path()' or
> 'strbuf_repo_git_path()' which will do the proper path adjustments when
> working on a path which should be shared between worktrees (i.e. part of
> the common git dir).

Thanks for the pointers, however, the above fix mirrors the fix made
by 744e469755 (clone: allow --local from a linked checkout,
2015-09-28) to code immediately below it in the 'else' arm:

get_common_dir(, src_repo);
get_common_dir(, dest_repo);
strbuf_addstr(, "/objects");
strbuf_addstr(, "/objects");

It would be poor form and confusing to use one of the mechanisms you
suggest while leaving the 'else' arm untouched.

Re-working both arms of the 'if' to use one of the suggested functions
would make a fine follow-on or preparatory patch, however, I'd rather
not hold up this fix merely to re-roll for such a minor cleanup. (I
also considered a follow-on patch to reduce the duplication between
the two cases but decided against it, for the present, since such a
patch would almost be noise without much gain.)

By the way, is there any documentation explaining the differences
between all these similar functions and when one should be used over
the others?

RE: [Proposed] Externalize man/html ref for quick-install-man and quick-install-html

[Randall S. Becker]

Sorry about the response positioning...

I can send you a pull request on github, if you want 

-Original Message-
From: git-ow...@vger.kernel.org [mailto:git-ow...@vger.kernel.org] On Behalf Of 
Junio C Hamano
Sent: December 11, 2017 6:27 PM
To: Randall S. Becker 
Cc: git@vger.kernel.org
Subject: Re: [Proposed] Externalize man/html ref for quick-install-man and 
quick-install-html

"Randall S. Becker"  writes:

> diff --git a/Documentation/Makefile b/Documentation/Makefile index 
> 3e39e28..4f1e6df 100644
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -39,6 +39,8 @@ MAN_TXT = $(MAN1_TXT) $(MAN5_TXT) $(MAN7_TXT)  
> MAN_XML = $(patsubst %.txt,%.xml,$(MAN_TXT))  MAN_HTML = $(patsubst 
> %.txt,%.html,$(MAN_TXT))
>
> +GIT_MAN_REF = master
> +
>  OBSOLETE_HTML += everyday.html
>  OBSOLETE_HTML += git-remote-helpers.html  DOC_HTML = $(MAN_HTML) 
> $(OBSOLETE_HTML) @@ -415,14 +417,14 @@ require-manrepo::
> then echo "git-manpages repository must exist at $(MAN_REPO)"; 
> exit 1; fi
>
>  quick-install-man: require-manrepo
> -   '$(SHELL_PATH_SQ)' ./install-doc-quick.sh $(MAN_REPO)
> $(DESTDIR)$(mandir)
> +   '$(SHELL_PATH_SQ)' ./install-doc-quick.sh $(MAN_REPO)
> $(DESTDIR)$(mandir) $(GIT_MAN_REF)

I suspect that this patch is line-wrapped and unusable for the final 
application, but I think what the change wants to do makes total sense---we are 
already letting the builder specify where the other repositories for docs are, 
and it is not such a big stretch to let them also say which branch or tag they 
want their documentation from.

Re: [PATCH 1/1] diffcore: add a filter to find a specific blob

[Stefan Beller]

On Mon, Dec 11, 2017 at 3:17 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> the information to what is shown. For example:
>>
>>   $ ./git log --oneline --blobfind=v2.0.0:Makefile
>>   b2feb64309 Revert the whole "ask curl-config" topic for now
>>   47fbfded53 i18n: only extract comments marked with "TRANSLATORS:"
>
> This part is a bit stale???

fixed in the next reroll :(

>> +--find-object=::
>> + Restrict the output such that one side of the diff
>> + matches the given object id. The object can be a blob,
>> + or gitlink entry.
>
> OK.  In principle you should also be able to find a tree, but I do
> not now how useful it would be.  Extending it to gitlink, which is
> another kind of leaf node in the reachability DAG, does make tons of
> sense---it's a no brainer that I feel ashamed not to have thought of
> myself ;-)

The current patch under discussion doesn't find trees, though.
Hence the documentation is accurate saying that only blobs and
gitlinks work.

>
>> +LIB_OBJS += diffcore-oidfind.o
>
> Just to nitpick, but "blobfind" was to find "blob", and if you are
> extending it to find any "object", then that should be "objfind".
> "oid" is _A_ way to refer to an object (i.e. the _name_ of it), and
> name is *not* the same as the thing the name refers to, so...

obj-find sounds good.

>
>> +static int parse_oidfind_opt(struct diff_options *opt, const char *arg)
>> +{
>> + struct object_id oid;
>> +
>> + /* We don't even need to have the object, any oid works. */
>> + if (get_oid_blob(arg, ))
>> + return error("unable to resolve '%s'", arg);
>
> Should this still be get_oid_blob(), or should it be less specific
> to blobs?

We could check if it is a tree/commit and die as they are not
being handled correctly.

Re: [PATCH] clone: support 'clone --shared' from a worktree

[Brandon Williams]

On 12/11, Eric Sunshine wrote:
> When worktree functionality was originally implemented, the possibility
> of 'clone --local' from within a worktree was overlooked, with the
> result that the location of the "objects" directory of the source
> repository was computed incorrectly, thus the objects could not be
> copied or hard-linked by the clone. This shortcoming was addressed by
> 744e469755 (clone: allow --local from a linked checkout, 2015-09-28).
> 
> However, the related case of 'clone --shared' (despite being handled
> only a few lines away from the 'clone --local' case) was not fixed by
> 744e469755, with a similar result of the "objects" directory location
> being incorrectly computed for insertion into the 'alternates' file.
> Fix this.
> 
> Reported-by: Marc-André Lureau 
> Signed-off-by: Eric Sunshine 
> ---
>  builtin/clone.c | 3 ++-
>  t/t2025-worktree-add.sh | 6 ++
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/clone.c b/builtin/clone.c
> index b22845738a..6ad0ab3fa4 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -452,7 +452,8 @@ static void clone_local(const char *src_repo, const char 
> *dest_repo)
>  {
>   if (option_shared) {
>   struct strbuf alt = STRBUF_INIT;
> - strbuf_addf(, "%s/objects", src_repo);
> + get_common_dir(, src_repo);
> + strbuf_addstr(, "/objects");

If you wanted to do this in one function call you could either use
'strbuf_git_common_path()' or either 'strbuf_git_path()' or
'strbuf_repo_git_path()' which will do the proper path adjustments when
working on a path which should be shared between worktrees (i.e. part of
the common git dir).

>   add_to_alternates_file(alt.buf);
>   strbuf_release();
>   } else {
> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> index b5c47ac602..7395973318 100755
> --- a/t/t2025-worktree-add.sh
> +++ b/t/t2025-worktree-add.sh
> @@ -245,6 +245,12 @@ test_expect_success 'local clone from linked checkout' '
>   ( cd here-clone && git fsck )
>  '
>  
> +test_expect_success 'local clone --shared from linked checkout' '
> + git -C bare worktree add --detach ../baretree &&
> + git clone --local --shared baretree bare-clone &&
> + grep /bare/ bare-clone/.git/objects/info/alternates
> +'
> +
>  test_expect_success '"add" worktree with --no-checkout' '
>   git worktree add --no-checkout -b swamp swamp &&
>   ! test -e swamp/init.t &&
> -- 
> 2.15.1.502.gccaef8de57
> 

-- 
Brandon Williams

Re: [PATCH] clone: support 'clone --shared' from a worktree

[Junio C Hamano]

Eric Sunshine  writes:

> When worktree functionality was originally implemented, the possibility
> of 'clone --local' from within a worktree was overlooked, with the
> result that the location of the "objects" directory of the source
> repository was computed incorrectly, thus the objects could not be
> copied or hard-linked by the clone. This shortcoming was addressed by
> 744e469755 (clone: allow --local from a linked checkout, 2015-09-28).
>
> However, the related case of 'clone --shared' (despite being handled
> only a few lines away from the 'clone --local' case) was not fixed by
> 744e469755, with a similar result of the "objects" directory location
> being incorrectly computed for insertion into the 'alternates' file.
> Fix this.
>
> Reported-by: Marc-André Lureau 
> Signed-off-by: Eric Sunshine 
> ---
>  builtin/clone.c | 3 ++-
>  t/t2025-worktree-add.sh | 6 ++
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index b22845738a..6ad0ab3fa4 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -452,7 +452,8 @@ static void clone_local(const char *src_repo, const char 
> *dest_repo)
>  {
>   if (option_shared) {
>   struct strbuf alt = STRBUF_INIT;
> - strbuf_addf(, "%s/objects", src_repo);
> + get_common_dir(, src_repo);
> + strbuf_addstr(, "/objects");
>   add_to_alternates_file(alt.buf);
>   strbuf_release();
>   } else {

Makes sense.  Will queue.

Thanks.

Re: [PATCH v1] convert: add support for 'encoding' attribute

[Eric Sunshine]

On Mon, Dec 11, 2017 at 6:47 PM, Lars Schneider
 wrote:
> On 11 Dec 2017, at 19:39, Eric Sunshine  wrote:
>> On Mon, Dec 11, 2017 at 10:50 AM,   wrote:
>>> From: Lars Schneider 
>>>
>>> Git and its tools (e.g. git diff) expect all text files in UTF-8
>>> encoding. Git will happily accept content in all other encodings, too,
>>> but it might not be able to process the text (e.g. viewing diffs or
>>> changing line endings).
>>>
>>> Add an attribute to tell Git what encoding the user has defined for a
>>> given file. If the content is added to the index, then Git converts the
>>> content to a canonical UTF-8 representation. On checkout Git will
>>> reverse the conversion.
>>>
>>> Reviewed-by: Patrick Lühne 
>>> Signed-off-by: Lars Schneider 
>>> ---
>>> +static int encode_to_git(const char *path, const char *src, size_t src_len,
>>> +struct strbuf *buf, struct encoding *enc)
>>> +{
>>> +   if (enc->to_git == invalid_conversion) {
>>> +   enc->to_git = iconv_open(default_encoding, encoding->name);
>>> +   if (enc->to_git == invalid_conversion)
>>> +   warning(_("unsupported encoding %s"), 
>>> encoding->name);
>>> +   }
>>> +
>>> +   if (enc->to_worktree == invalid_conversion)
>>> +   enc->to_worktree = iconv_open(encoding->name, 
>>> default_encoding);
>>
>> Do you need to be calling iconv_close() somewhere on the result of the
>> iconv_open() calls? [Answering myself after reading the rest of the
>> patch: You're caching these opened 'iconv' descriptors, so you don't
>> plan on closing them.]
>
> Should this information go into the commit message to avoid confusing
> future readers? I think, yes.

Maybe. However, the code which does the actual caching is so distant
from these iconv_open() invocations that it might be more helpful to
have an in-code comment here saying that the "missing" iconv_close()
invocations is intentional.

Re: [PATCH v1] convert: add support for 'encoding' attribute

[Lars Schneider]

On 11 Dec 2017, at 19:39, Eric Sunshine  wrote:

> On Mon, Dec 11, 2017 at 10:50 AM,   wrote:
>> From: Lars Schneider 
>> 
>> Git and its tools (e.g. git diff) expect all text files in UTF-8
>> encoding. Git will happily accept content in all other encodings, too,
>> but it might not be able to process the text (e.g. viewing diffs or
>> changing line endings).
>> 
>> Add an attribute to tell Git what encoding the user has defined for a
>> given file. If the content is added to the index, then Git converts the
>> content to a canonical UTF-8 representation. On checkout Git will
>> reverse the conversion.
>> 
>> Reviewed-by: Patrick Lühne 
>> Signed-off-by: Lars Schneider 
>> ---
>> diff --git a/convert.c b/convert.c
>> @@ -256,6 +257,149 @@ static int will_convert_lf_to_crlf(size_t len, struct 
>> text_stat *stats,
>> +static int encode_to_git(const char *path, const char *src, size_t src_len,
>> +struct strbuf *buf, struct encoding *enc)
>> +{
>> +#ifndef NO_ICONV
>> +   char *dst, *re_src;
>> +   int dst_len, re_src_len;
>> +
>> +   /*
>> +* No encoding is specified or there is nothing to encode.
>> +* Tell the caller that the content was not modified.
>> +*/
>> +   if (!enc || (src && !src_len))
>> +   return 0;
>> +
>> +   /*
>> +* Looks like we got called from "would_convert_to_git()".
>> +* This means Git wants to know if it would encode (= modify!)
>> +* the content. Let's answer with "yes", since an encoding was
>> +* specified.
>> +*/
>> +   if (!buf && !src)
>> +   return 1;
>> +
>> +   if (enc->to_git == invalid_conversion) {
>> +   enc->to_git = iconv_open(default_encoding, encoding->name);
>> +   if (enc->to_git == invalid_conversion)
>> +   warning(_("unsupported encoding %s"), 
>> encoding->name);
>> +   }
>> +
>> +   if (enc->to_worktree == invalid_conversion)
>> +   enc->to_worktree = iconv_open(encoding->name, 
>> default_encoding);
> 
> Do you need to be calling iconv_close() somewhere on the result of the
> iconv_open() calls? [Answering myself after reading the rest of the
> patch: You're caching these opened 'iconv' descriptors, so you don't
> plan on closing them.]

Should this information go into the commit message to avoid confusing
future readers? I think, yes.


>> + [...]
>> +   /*
>> +* Encode dst back to ensure no information is lost. This wastes
>> +* a few cycles as most conversions are round trip conversion
>> +* safe. However, content that has an invalid encoding might not
>> +* match its original byte sequence after the UTF-8 conversion
>> +* round trip. Let's play safe here and check the round trip
>> +* conversion.
>> +*/
>> +   re_src = reencode_string_iconv(dst, dst_len, enc->to_worktree, 
>> _src_len);
>> +   if (!re_src || strcmp(src, re_src)) {
> 
> You're using strcmp() as opposed to memcmp() because you expect
> 're_src' will unconditionally be UTF-8-encoded, right?

Yes. But since you mention it, I think it would be better to use
memcmp() here! Plus, it might be a tiny bit faster ;-)

Thanks,
Lars

Re: [PATCH v5 0/9] sequencer: don't fork git commit

[Junio C Hamano]

Phillip Wood  writes:

> From: Phillip Wood 
>
> I've reworked the config handling since v4. It now stores the default
> values in struct replay_opt rather than using global variables and
> calls git_diff_basic_config(). Unfortunately I've not had time to
> modify git_gpg_config() to indicate if it successfully handled the key
> so git_diff_basic_config() is called unnecessarily in that case. Within
> git_diff_basic_config() userdiff_config() also suffers from the same
> problem of not indicating if it has handled the key.

Ouch.  I thought we agreed that we were ready to go incremental and
the topic was merged to 'next' earlier last week.

After scanning the difference between the two rounds, it seems that
the more important difference is the rework of the configuration,
which looks better thought out than the previous round, and with
associated change to use replay_opts fields instead of free variables
to carry gpg-sign and cleanup-mode settings around, which also looks
sensible to me.

Can you make these differences into incremental "that earlier one
was suboptimal for this and that reasons, let's make it better by
doing this" patches to queue them on top?

Thanks.

Re: [PATCH v1] convert: add support for 'encoding' attribute

[Lars Schneider]

On 11 Dec 2017, at 21:47, Johannes Sixt  wrote:

> Am 11.12.2017 um 16:50 schrieb lars.schnei...@autodesk.com:
>> From: Lars Schneider 
>> Git and its tools (e.g. git diff) expect all text files in UTF-8
>> encoding. Git will happily accept content in all other encodings, too,
>> but it might not be able to process the text (e.g. viewing diffs or
>> changing line endings).
>> Add an attribute to tell Git what encoding the user has defined for a
>> given file. If the content is added to the index, then Git converts the
>> content to a canonical UTF-8 representation. On checkout Git will
>> reverse the conversion.
>> Reviewed-by: Patrick Lühne 
>> Signed-off-by: Lars Schneider 
>> ---
>> Hi,
>> here is a WIP patch to add text encoding support for files encoded with
>> something other than UTF-8 [RFC].
>> The 'encoding' attribute is already used to view blobs in gitk. That
>> could be a problem as the content is stored in Git with the defined
>> encoding. This patch would interpret the content as UTF-8 encoded and
> 
> This will be a major drawback for me because my code base stores text files 
> that are not UTF-8 encoded. And I do use the existing 'encoding' attribute to 
> view the text in git-gui and gitk. Repurposing this attribute name is not an 
> option, IMO.

I understand your point of view and I kind of expected that that reply.
Thanks for the feedback!

Question is: Given that "encoding" is not available, how could I name
 the attribute without confusing the user?

I contemplated:
  - "enc" or "encode" because "eol" and "ident" use abbreviations, too
(enc could be confused with encryption. plus, a user might ask
 what is the difference between "enc" and "encoding" attribute :-)
  - "wte", "wtenc", or "worktree-encoding" to emphasize that this is 
the encoding used in the worktree 
(I fear that users think that is git-worktree, the command, related)

I think my favorite is "worktree-encoding".
What do you think?

Thanks,
Lars 


BTW: I am curios, can you share what encoding you use?
My main use case is UTF-16 and I was surprised that I haven't
found a single public repo on github.com with "encoding=utf-16"

[PATCH 4/4] travis-ci: set GIT_TEST_HTTPD in 'ci/lib-travisci.sh'

[SZEDER Gábor]

Commit 657343a60 (travis-ci: move Travis CI code into dedicated
scripts, 2017-09-10) converted '.travis.yml's default 'before_install'
scriptlet to the 'ci/install-dependencies.sh' script, and while doing
so moved setting GIT_TEST_HTTPD=YesPlease for the 64-bit GCC and Clang
Linux build jobs to that script.  This is wrong for two reasons:

 - The purpose of that script is, as its name suggests, to install
   dependencies, not to set any environment variables influencing
   which tests should be run (though, arguably, this was already an
   issue with the original 'before_install' scriptlet).

 - Setting the variable has no effect anymore, because that script is
   run in a separate shell process, and the variable won't be visible
   in any of the other scripts, notably in 'ci/run-tests.sh'
   responsible for, well, running the tests.

Luckily, this didn't have a negative effect on our Travis CI build
jobs, because GIT_TEST_HTTPD is a tri-state variable defaulting to
"auto" and a functioning web server was installed in those Linux build
jobs, so the httpd tests were run anyway.

Apparently the httpd tests run just fine without GIT_TEST_HTTPD being
set, therefore we could simply remove this environment variable.
However, if a bug were to creep in to change the Travis CI build
environment to run the tests as root or to not install Apache, then
the httpd tests would be skipped and the build job would still
succeed.  We would only notice if someone actually were to look
through the build job's trace log; but who would look at the trace log
of a successful build job?!

Since httpd tests are important, we do want to run them and we want to
be loudly reminded if they can't be run.  Therefore, move setting
GIT_TEST_HTTPD=YesPlease for the 64-bit GCC and Clang Linux build jobs
to 'ci/lib-travisci.sh' to ensure that the build job fails when the
httpd tests can't be run.  (We could set it in 'ci/run-tests.sh' just
as well, but it's better to keep all environment variables in one
place in 'ci/lib-travisci.sh'.)

Signed-off-by: SZEDER Gábor 
---
 ci/install-dependencies.sh | 2 --
 ci/lib-travisci.sh | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 468788566..75a9fd247 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -10,8 +10,6 @@ 
LFSWHENCE=https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VE
 
 case "$jobname" in
 linux-clang|linux-gcc)
-   export GIT_TEST_HTTPD=YesPlease
-
mkdir --parents "$P4_PATH"
pushd "$P4_PATH"
wget --quiet "$P4WHENCE/bin.linux26x86_64/p4d"
diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
index e85571298..331d3eb3a 100755
--- a/ci/lib-travisci.sh
+++ b/ci/lib-travisci.sh
@@ -40,6 +40,8 @@ export GIT_TEST_CLONE_2GB=YesPlease
 
 case "$jobname" in
 linux-clang|linux-gcc)
+   export GIT_TEST_HTTPD=YesPlease
+
# The Linux build installs the defined dependency versions below.
# The OS X build installs the latest available versions. Keep that
# in mind when you encounter a broken OS X build!
-- 
2.15.1.421.gc469ca1de

[PATCH 1/4] travis-ci: use 'set -x' in 'ci/*' scripts for extra tracing output

[SZEDER Gábor]

While the build logic was embedded in our '.travis.yml', Travis CI
used to produce a nice trace log including all commands executed in
those embedded scriptlets.  Since 657343a60 (travis-ci: move Travis CI
code into dedicated scripts, 2017-09-10), however, we only see the
name of the dedicated scripts, but not what those scripts are actually
doing, resulting in a less useful trace log.  A patch later in this
series will move setting environment variables from '.travis.yml' to
the 'ci/*' scripts, so not even those will be included in the trace
log.

Use 'set -x' in 'ci/lib-travisci.sh', which is sourced in most other
'ci/*' scripts, so we get trace log about the commands executed in all
of those scripts.

Signed-off-by: SZEDER Gábor 
---
 ci/lib-travisci.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
index ac05f1f46..a0c8ae03f 100755
--- a/ci/lib-travisci.sh
+++ b/ci/lib-travisci.sh
@@ -23,7 +23,7 @@ skip_branch_tip_with_tag () {
 
 # Set 'exit on error' for all CI scripts to let the caller know that
 # something went wrong
-set -e
+set -ex
 
 skip_branch_tip_with_tag
 
-- 
2.15.1.421.gc469ca1de

[PATCH 3/4] travis-ci: move setting environment variables to 'ci/lib-travisci.sh'

[SZEDER Gábor]

Our '.travis.yml's 'env.global' section sets a bunch of environment
variables for all build jobs, though none of them actually affects all
build jobs.  It's convenient for us, and in most cases it works just
fine, because irrelevant environment variables are simply ignored.

However, $GIT_SKIP_TESTS is an exception: it tells the test harness to
skip the two test scripts that are prone to occasional failures on
OSX, but as it's set for all build jobs those tests are not run in any
of the build jobs that are capable to run them reliably, either.

Therefore $GIT_SKIP_TESTS should only be set in the OSX build jobs,
but those build jobs are included in the build matrix implicitly (i.e.
by combining the matrix keys 'os' and 'compiler'), and there is no way
to set an environment variable only for a subset of those implicit
build jobs.  (Unless we were to add new scriptlets to '.travis.yml',
which is exactly the opposite direction that we took with commit
657343a60 (travis-ci: move Travis CI code into dedicated scripts,
2017-09-10)).

So move setting $GIT_SKIP_TESTS to 'ci/lib-travisci.sh', where it can
trivially be set only for the OSX build jobs.

Furthermore, move setting all other environment variables from
'.travis.yml' to 'ci/lib-travisci.sh', too, because a couple of
environment variables are already set there, and this way all
environment variables will be set in the same place.  All the logic
controlling our builds is already in the 'ci/*' scripts anyway, so
there is really no good reason to keep the environment variables
separately.

Signed-off-by: SZEDER Gábor 
---
 .travis.yml| 18 +-
 ci/lib-travisci.sh | 21 +
 2 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index 88435e11c..7c9aa0557 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -21,25 +21,9 @@ addons:
 - git-svn
 - apache2
 
-env:
-  global:
-- DEVELOPER=1
-# The Linux build installs the defined dependency versions below.
-# The OS X build installs the latest available versions. Keep that
-# in mind when you encounter a broken OS X build!
-- LINUX_P4_VERSION="16.2"
-- LINUX_GIT_LFS_VERSION="1.5.2"
-- DEFAULT_TEST_TARGET=prove
-- GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
-- GIT_TEST_OPTS="--verbose-log"
-- GIT_TEST_CLONE_2GB=YesPlease
-# t9810 occasionally fails on Travis CI OS X
-# t9816 occasionally fails with "TAP out of sequence errors" on Travis CI 
OS X
-- GIT_SKIP_TESTS="t9810 t9816"
-
 matrix:
   include:
-- env: jobname=GETTEXT_POISON GETTEXT_POSION=YesPlease
+- env: jobname=GETTEXT_POISON
   os: linux
   compiler:
   addons:
diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
index b60e93797..e85571298 100755
--- a/ci/lib-travisci.sh
+++ b/ci/lib-travisci.sh
@@ -32,10 +32,31 @@ then
jobname="$TRAVIS_OS_NAME-$CC"
 fi
 
+export DEVELOPER=1
+export DEFAULT_TEST_TARGET=prove
+export GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
+export GIT_TEST_OPTS="--verbose-log"
+export GIT_TEST_CLONE_2GB=YesPlease
+
 case "$jobname" in
 linux-clang|linux-gcc)
+   # The Linux build installs the defined dependency versions below.
+   # The OS X build installs the latest available versions. Keep that
+   # in mind when you encounter a broken OS X build!
+   export LINUX_P4_VERSION="16.2"
+   export LINUX_GIT_LFS_VERSION="1.5.2"
+
P4_PATH="$(pwd)/custom/p4"
GIT_LFS_PATH="$(pwd)/custom/git-lfs"
export PATH="$GIT_LFS_PATH:$P4_PATH:$PATH"
;;
+osx-clang|osx-gcc)
+   # t9810 occasionally fails on Travis CI OS X
+   # t9816 occasionally fails with "TAP out of sequence errors" on
+   # Travis CI OS X
+   export GIT_SKIP_TESTS="t9810 t9816"
+   ;;
+GETTEXT_POISON)
+   export GETTEXT_POISON=YesPlease
+   ;;
 esac
-- 
2.15.1.421.gc469ca1de

[PATCH 0/4] travis-ci: clean up setting environment variables

[SZEDER Gábor]

(Was: [PATCH] travis-ci: fix running P4 and Git LFS tests in Linux build
jobs)

On Wed, Nov 1, 2017 at 12:55 PM, SZEDER Gábor  wrote:
> However, after these
> embedded scriptlets were moved into dedicated scripts executed in
> separate shell processes, any variable set in one of those scripts is
> only visible in that single script but not in any of the others.

> diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> index a29246af3..5bd06fe90 100755
> --- a/ci/install-dependencies.sh
> +++ b/ci/install-dependencies.sh
> @@ -12,20 +12,18 @@ case "${TRAVIS_OS_NAME:-linux}" in
>  linux)
> export GIT_TEST_HTTPD=YesPlease

Astute readers ;) might have been wondering what's the deal with this
environment variable in the patch context, since this won't have any
effect outside of this script, either.  Alas, it's not as easy as moving
setting GIT_TEST_HTTPD to 'ci/lib-travisci.sh', like the quoted patch
did with paths to P4 and Git LFS, because then it would be set for the 32
bit Linux build job which runs everything as root, thus can't run https
tests.

This patch series deals with this by adding means that 'ci/*' scripts
can use to identify which build job they are taking part in (patch 3),
and then setting environment variables in 'ci/lib-travisci.sh', where we
have now more freedom to set a specific variable only for specific build
jobs (patches 3 and 4).


This patch series conflicts with the last patch in Thomas' split index
fix series.

  https://public-inbox.org/git/20171210212202.28231-4-t.gumme...@gmail.com/

The conflict is not overly difficult, but to resolve it we should first
come up with a reasonable job name for that build job, e.g. something
like "misc-knobs" or whatever, because the combination
"GETTEXT_POISON-SPLIT_INDEX" is just too long to exist and won't scale
if we enable further knobs in the future.


SZEDER Gábor (4):
  travis-ci: use 'set -x' in 'ci/*' scripts for extra tracing output
  travis-ci: introduce a $jobname variable for 'ci/*' scripts
  travis-ci: move setting environment variables to 'ci/lib-travisci.sh'
  travis-ci: set GIT_TEST_HTTPD in 'ci/lib-travisci.sh'

 .travis.yml| 26 +-
 ci/install-dependencies.sh |  8 +++-
 ci/lib-travisci.sh | 34 +++---
 3 files changed, 39 insertions(+), 29 deletions(-)

-- 
2.15.1.421.gc469ca1de

[PATCH 2/4] travis-ci: introduce a $jobname variable for 'ci/*' scripts

[SZEDER Gábor]

A couple of 'ci/*' scripts are shared between different build jobs:
'ci/lib-travisci.sh', being a common library, is sourced from almost
every script, while 'ci/install-dependencies.sh', 'ci/run-build.sh'
and 'ci/run-tests.sh' are shared between the "regular" GCC and Clang
Linux and OSX build jobs, and the latter two scripts are used in the
GETTEXT_POISON Linux build job as well.

Our builds could benefit from these shared scripts being able to
easily tell which build job they are taking part in.  Now, it's
already quite easy to tell apart Linux vs OSX and GCC vs Clang build
jobs, but it gets trickier with all the additional Linux-based build
jobs included explicitly in the build matrix.

Unfortunately, Travis CI doesn't provide much help in this regard.
The closest we've got is the $TRAVIS_JOB_NUMBER variable, the value of
which is two dot-separated integers, where the second integer
indicates a particular build job.  While it would be possible to use
that second number to identify the build job in our shared scripts, it
doesn't seem like a good idea to rely on that:

  - Though the build job numbering sequence seems to be stable so far,
Travis CI's documentation doesn't explicitly states that it is
indeed stable and will remain so in the future.  And even if it
were stable,

  - if we were to remove or insert a build job in the middle, then the
job numbers of all subsequent build jobs would change accordingly.

So roll our own means of simple build job identification and introduce
the $jobname environment variable in our builds, setting it in the
environments of the explicitly included jobs in '.travis.yml', while
constructing one in 'ci/lib-travisci.sh' as the combination of the OS
and compiler name for the GCC and Clang Linux and OSX build jobs.  Use
$jobname instead of $TRAVIS_OS_NAME in scripts taking different
actions based on the OS and build job (when installing P4 and Git LFS
dependencies and including them in $PATH).  The following two patches
will also rely on $jobname.

Signed-off-by: SZEDER Gábor 
---
 .travis.yml| 10 +-
 ci/install-dependencies.sh |  6 +++---
 ci/lib-travisci.sh |  9 +++--
 3 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index 281f101f3..88435e11c 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -39,12 +39,12 @@ env:
 
 matrix:
   include:
-- env: GETTEXT_POISON=YesPlease
+- env: jobname=GETTEXT_POISON GETTEXT_POSION=YesPlease
   os: linux
   compiler:
   addons:
   before_install:
-- env: Windows
+- env: jobname=Windows
   os: linux
   compiler:
   addons:
@@ -55,7 +55,7 @@ matrix:
   test "$TRAVIS_REPO_SLUG" != "git/git" ||
   ci/run-windows-build.sh $TRAVIS_BRANCH $(git rev-parse HEAD)
   after_failure:
-- env: Linux32
+- env: jobname=Linux32
   os: linux
   compiler:
   services:
@@ -63,7 +63,7 @@ matrix:
   before_install:
   before_script:
   script: ci/run-linux32-docker.sh
-- env: Static Analysis
+- env: jobname=StaticAnalysis
   os: linux
   compiler:
   addons:
@@ -74,7 +74,7 @@ matrix:
   before_script:
   script: ci/run-static-analysis.sh
   after_failure:
-- env: Documentation
+- env: jobname=Documentation
   os: linux
   compiler:
   addons:
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 5bd06fe90..468788566 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -8,8 +8,8 @@
 P4WHENCE=http://filehost.perforce.com/perforce/r$LINUX_P4_VERSION
 
LFSWHENCE=https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VERSION
 
-case "${TRAVIS_OS_NAME:-linux}" in
-linux)
+case "$jobname" in
+linux-clang|linux-gcc)
export GIT_TEST_HTTPD=YesPlease
 
mkdir --parents "$P4_PATH"
@@ -26,7 +26,7 @@ linux)
cp git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs .
popd
;;
-osx)
+osx-clang|osx-gcc)
brew update --quiet
# Uncomment this if you want to run perf tests:
# brew install gnu-time
diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
index a0c8ae03f..b60e93797 100755
--- a/ci/lib-travisci.sh
+++ b/ci/lib-travisci.sh
@@ -27,8 +27,13 @@ set -ex
 
 skip_branch_tip_with_tag
 
-case "${TRAVIS_OS_NAME:-linux}" in
-linux)
+if test -z "$jobname"
+then
+   jobname="$TRAVIS_OS_NAME-$CC"
+fi
+
+case "$jobname" in
+linux-clang|linux-gcc)
P4_PATH="$(pwd)/custom/p4"
GIT_LFS_PATH="$(pwd)/custom/git-lfs"
export PATH="$GIT_LFS_PATH:$P4_PATH:$PATH"
-- 
2.15.1.421.gc469ca1de

Re: [PATCH v3] Makefile: replace perl/Makefile.PL with simple make rules

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> I don't know what I was thinking when I wrote this, but this logic
> should be fully robust, and I've confirmed that all tests pass
> with/without an Error.pm installed globally.

Thanks.  Will queue and drop the revert from 'pu'.

Re: [Proposed] Externalize man/html ref for quick-install-man and quick-install-html

[Junio C Hamano]

"Randall S. Becker"  writes:

> diff --git a/Documentation/Makefile b/Documentation/Makefile
> index 3e39e28..4f1e6df 100644
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -39,6 +39,8 @@ MAN_TXT = $(MAN1_TXT) $(MAN5_TXT) $(MAN7_TXT)
>  MAN_XML = $(patsubst %.txt,%.xml,$(MAN_TXT))
>  MAN_HTML = $(patsubst %.txt,%.html,$(MAN_TXT))
>
> +GIT_MAN_REF = master
> +
>  OBSOLETE_HTML += everyday.html
>  OBSOLETE_HTML += git-remote-helpers.html
>  DOC_HTML = $(MAN_HTML) $(OBSOLETE_HTML)
> @@ -415,14 +417,14 @@ require-manrepo::
> then echo "git-manpages repository must exist at $(MAN_REPO)"; exit
> 1; fi
>
>  quick-install-man: require-manrepo
> -   '$(SHELL_PATH_SQ)' ./install-doc-quick.sh $(MAN_REPO)
> $(DESTDIR)$(mandir)
> +   '$(SHELL_PATH_SQ)' ./install-doc-quick.sh $(MAN_REPO)
> $(DESTDIR)$(mandir) $(GIT_MAN_REF)

I suspect that this patch is line-wrapped and unusable for the final
application, but I think what the change wants to do makes total
sense---we are already letting the builder specify where the other
repositories for docs are, and it is not such a big stretch to let
them also say which branch or tag they want their documentation
from.

Re: [PATCH v3 1/7] git-compat-util: introduce skip_to_optional_arg()

[Junio C Hamano]

Jeff King  writes:

>
>   +   else if (starts_with(arg, "-B") ||
>   +skip_to_optional_arg(arg, "--break-rewrites", NULL)) {
> if ((options->break_opt = diff_scoreopt_parse(arg)) == -1)
>
> So that's kind-of weird, because we are parsing "-B", etc, and then
> expecting it to be _reparsed_ by diff_scoreopt_parse. So the two
> callsites must always match.

Correct.  diff_scoreopt_parse() can be coaxed to fit better within
this if/else if/... cascade by making it take a pointer to .break_opt
field and have it return "did I handle the -B/break-rewrites?" etc.,
but otherwise, this shows that skip_to_optional_arg() has impedance
mismatch with its current API.  And the NULL thing serves as a reminder
that skip_to_optional_arg() is used _only_ as a prefix check and not
for parsing.

Re: [PATCH 1/1] diffcore: add a filter to find a specific blob

[Junio C Hamano]

Stefan Beller  writes:

> the information to what is shown. For example:
>
>   $ ./git log --oneline --blobfind=v2.0.0:Makefile
>   b2feb64309 Revert the whole "ask curl-config" topic for now
>   47fbfded53 i18n: only extract comments marked with "TRANSLATORS:"

This part is a bit stale???

> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index dd0dba5b1d..67a99e522b 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -500,6 +500,12 @@ information.
>  --pickaxe-regex::
>   Treat the  given to `-S` as an extended POSIX regular
>   expression to match.
> +
> +--find-object=::
> + Restrict the output such that one side of the diff
> + matches the given object id. The object can be a blob,
> + or gitlink entry.

OK.  In principle you should also be able to find a tree, but I do
not now how useful it would be.  Extending it to gitlink, which is
another kind of leaf node in the reachability DAG, does make tons of
sense---it's a no brainer that I feel ashamed not to have thought of
myself ;-)

> +LIB_OBJS += diffcore-oidfind.o

Just to nitpick, but "blobfind" was to find "blob", and if you are
extending it to find any "object", then that should be "objfind".
"oid" is _A_ way to refer to an object (i.e. the _name_ of it), and
name is *not* the same as the thing the name refers to, so...

> +static int parse_oidfind_opt(struct diff_options *opt, const char *arg)
> +{
> + struct object_id oid;
> +
> + /* We don't even need to have the object, any oid works. */
> + if (get_oid_blob(arg, ))
> + return error("unable to resolve '%s'", arg);

Should this still be get_oid_blob(), or should it be less specific
to blobs?

> +test_expect_success 'find the greeting blob' '
> + cat >expect <<-EOF &&
> + Revert "add the greeting blob"
> + add the greeting blob
> + EOF
> +
> + git log --format=%s --find-object=greeting^{blob} >actual &&
> +
> + test_cmp expect actual
> +'

Makes sense.

> +
> +test_expect_success 'setup a submodule' '
> + test_create_repo sub &&
> + test_commit -C sub sub &&
> + git submodule add ./sub sub &&
> + git commit -a -m "add sub"
> +'
> +
> +test_expect_success 'find a submodule' '
> + cat >expect <<-EOF &&
> + add sub
> + EOF
> +
> + git log --format=%s --find-object=HEAD:sub >actual &&
> +
> + test_cmp expect actual
> +'

Nice (and cute).

> +test_done

Looking good.  Thanks.

[PATCH] clone: support 'clone --shared' from a worktree

[Eric Sunshine]

When worktree functionality was originally implemented, the possibility
of 'clone --local' from within a worktree was overlooked, with the
result that the location of the "objects" directory of the source
repository was computed incorrectly, thus the objects could not be
copied or hard-linked by the clone. This shortcoming was addressed by
744e469755 (clone: allow --local from a linked checkout, 2015-09-28).

However, the related case of 'clone --shared' (despite being handled
only a few lines away from the 'clone --local' case) was not fixed by
744e469755, with a similar result of the "objects" directory location
being incorrectly computed for insertion into the 'alternates' file.
Fix this.

Reported-by: Marc-André Lureau 
Signed-off-by: Eric Sunshine 
---
 builtin/clone.c | 3 ++-
 t/t2025-worktree-add.sh | 6 ++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index b22845738a..6ad0ab3fa4 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -452,7 +452,8 @@ static void clone_local(const char *src_repo, const char 
*dest_repo)
 {
if (option_shared) {
struct strbuf alt = STRBUF_INIT;
-   strbuf_addf(, "%s/objects", src_repo);
+   get_common_dir(, src_repo);
+   strbuf_addstr(, "/objects");
add_to_alternates_file(alt.buf);
strbuf_release();
} else {
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index b5c47ac602..7395973318 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -245,6 +245,12 @@ test_expect_success 'local clone from linked checkout' '
( cd here-clone && git fsck )
 '
 
+test_expect_success 'local clone --shared from linked checkout' '
+   git -C bare worktree add --detach ../baretree &&
+   git clone --local --shared baretree bare-clone &&
+   grep /bare/ bare-clone/.git/objects/info/alternates
+'
+
 test_expect_success '"add" worktree with --no-checkout' '
git worktree add --no-checkout -b swamp swamp &&
! test -e swamp/init.t &&
-- 
2.15.1.502.gccaef8de57

Read

[Ella Golan]

I am Ms.Ella Golan, I am the Executive Vice President Banking Division with
FIRST INTERNATIONAL BANK OF ISRAEL LTD (FIBI).
I am getting in touch with you regarding an extremely important and urgent
matter. If you would oblige me the opportunity, I shall provide you with

details upon your Response

Faithfully,
Ms.Ella Golan

Re: [PATCH v1 1/1] check-non-portable-shell.pl: Quoted `wc -l` is not portable

[Junio C Hamano]

tbo...@web.de writes:

> From: Torsten Bögershausen 
>
> wc -l is used to count the number if lines in test scripts.

H, "is used" -> "should not be used", probably.

Just say "Use test_line_count instead" and leave the explanation why
it is a better thing to use to fb3340a6 ("test-lib: introduce
test_line_count to measure files", 2010-10-31), perhaps?

Re: Q: rational for $XDG_CONFIG_HOME/git/config to be "non global" or just a bug?

[Jonathan Nieder]

Hi,

Yaroslav Halchenko wrote:

> Example to show that TFM outlines precedence and --global correctly:
>
> $> grep xdg .gitconfig .config/git/config
> .gitconfig:xdg-and-user = user
> .config/git/config: xdg = xdg
> .config/git/config: xdg-and-user = xdg
> $> git config user.xdg ; git config user.xdg-and-user
> xdg
> user

I agree, this is confusing.

Reverse engineering from source, I find that git reads the following
files in sequence:

system:
/etc/gitconfig
global:
$XDG_CONFIG_HOME/git/config
$HOME/.gitconfig
repo:
$GIT_DIR/config
commandline:
options passed with -c or GIT_CONFIG_PARAMETERS

These terms (system, global, repo, etc) are accessible in code as
current_config_scope().  I don't think there's any user-visible effect
to $XDG_CONFIG_HOME/git/config and $HOME/.gitconfig both being global
--- it would probably be a good cleanup to rename the scope for one of
them.

I think the documentation

~/.gitconfig
User-specific configuration file. Also called "global"
configuration file.

should be clarified --- e.g. it could say

$XDG_CONFIG_HOME/git/config
~/.gitconfig
User-specific configuration files. Because options in
these files are not specific to any repository, thes
are sometimes called global configuration files.

As for "git config --global", I think the best thing would be to split
it into two options: something like "git config --user" and "git
config --xdg-user".  That way, it is unambiguous which configuration
file the user intends to inspect or modify.  When a user calls "git
config --global" and both files exist, it could warn that the command
is ambiguous.

Thoughts?

Thanks,
Jonathan

Re: [PATCH 3/3] travis: run tests with GIT_TEST_SPLIT_INDEX

[Thomas Gummerer]

On 12/11, SZEDER Gábor wrote:
> > Make sure that split index doesn't get broken, by running it on travis
> > CI.
> > 
> > Run the test suite with split index enabled in linux 64 bit mode, and
> > leave split index turned off in 32-bit mode.
> 
> This doesn't accurately describe what the patch does.
> Travis CI runs three 64 bit Linux build jobs for us: one compiled with
> Clang, one with GCC, and one with GETTEXT_POISON enabled.  This patch
> enables split index only in the latter build job, but not in the Clang
> and GCC build jobs.

You're right, it's my first time using travis CI and I got confused
about how the .travis.yml works, thanks for catching that.  Will
re-phrase the commit message.

> >  The laternative would be
> > to add an extra target in the matrix, enabling split index mode, but
> > that would only use additional cycles on travis and would not bring that
> > much benefit, as we are still running the test suite in the "vanilla"
> > version in the 32-bit mode.
> > 
> > Signed-off-by: Thomas Gummerer 
> > ---
> >  .travis.yml | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/.travis.yml b/.travis.yml
> > index 281f101f31..c83c766dee 100644
> > --- a/.travis.yml
> > +++ b/.travis.yml
> > @@ -39,7 +39,7 @@ env:
> >  
> >  matrix:
> >include:
> > -- env: GETTEXT_POISON=YesPlease
> > +- env: GETTEXT_POISON=YesPlease GIT_TEST_SPLIT_INDEX=YesPlease
> >os: linux
> >compiler:
> >addons:
> > -- 
> > 2.15.1.504.g5279b80103

Re: [PATCH 2/3] prune: fix pruning with multiple worktrees and split index

[Thomas Gummerer]

On 12/11, Brandon Williams wrote:
> On 12/10, Thomas Gummerer wrote:
> > be489d02d2 ("revision.c: --indexed-objects add objects from all
> > worktrees", 2017-08-23) made sure that pruning takes objects from all
> > worktrees into account.
> > 
> > It did that by reading the index of every worktree and adding the
> > necessary index objects to the set of pending objects.  The index is
> > read by read_index_from.  As mentioned in the previous commit,
> > read_index_from depends on the CWD for the location of the split index,
> > and add_index_objects_to_pending doesn't set that before using
> > read_index_from.
> > 
> > Instead of using read_index_from, use repo_read_index, which is aware of
> > the proper paths for the worktree.
> > 
> > This fixes t5304-prune when ran with GIT_TEST_SPLIT_INDEX set.
> > 
> 
> I'm on the fence about this change.  I understand that this will ensure
> that the proper objects aren't pruned when using a split index in the
> presence of worktrees but I think the solution needs to be thought
> through a bit more.
> 
> My big concern right now is the interaction of 'struct worktree's and
> 'struct repository'.  I'll try to highlight my concerns below.

Thanks for the review!  My thoughts on the concerns are below.

> > Signed-off-by: Thomas Gummerer 
> > ---
> > 
> > This also fixes t7009 when ran with GIT_TEST_SPLIT_INDEX.  I'm not
> > quite sure why it is fixed by this.  Either way I tracked the failure
> > down to f767178a5a ("Merge branch 'jk/no-null-sha1-in-cache-tree'",
> > 2017-05-16).  Maybe Peff has an idea why this fixes that test?
> > 
> >  repository.c | 11 +++
> >  repository.h |  2 ++
> >  revision.c   | 13 -
> >  3 files changed, 21 insertions(+), 5 deletions(-)
> > 
> > diff --git a/repository.c b/repository.c
> > index 928b1f553d..3c9bfbd1b8 100644
> > --- a/repository.c
> > +++ b/repository.c
> > @@ -2,6 +2,7 @@
> >  #include "repository.h"
> >  #include "config.h"
> >  #include "submodule-config.h"
> > +#include "worktree.h"
> >  
> >  /* The main repository */
> >  static struct repository the_repo = {
> > @@ -146,6 +147,16 @@ int repo_init(struct repository *repo, const char 
> > *gitdir, const char *worktree)
> > return -1;
> >  }
> >  
> > +/*
> > + * Initialize 'repo' based on the provided worktree
> > + * Return 0 upon success and a non-zero value upon failure.
> > + */
> > +int repo_worktree_init(struct repository *repo, struct worktree *worktree)
> > +{
> > +   return repo_init(repo, get_worktree_git_dir(worktree),
> > +worktree->path);
> > +}
> 
> My first concern is the use of 'get_worktree_git_dir()'.  Under the hood
> it calls 'get_git_dir()', 'get_git_common_dir()', and
> 'git_common_path()' which rely on global state as stored in
> 'the_repository'.  So how does one initialize a repository struct (using
> this initializer) using a worktree from a repository other than the
> global 'the_repository' struct?  I'm not sure I have an answer right
> now, but its an issue that needs to be thought through before we head
> down this road.

The main reason for just using 'get_worktree_git_dir()' was because it
exists and it does exactly what I needed.  If we ever have the need to
initialize a repository struct for a worktree from another repository
struct, I would imagine we introduce a new function
'repo_get_worktree_git_dir()', which takes a struct repository as
input and returns the worktree git dir based on that.

And then we'd have to add a different initializer
'repo_worktree_init_from_repo()' or something like that, where we
could initialize a worktree from a repository struct different from
'the_repository'.  But maybe there are some complications there that I
didn't think about?

> Just thinking to myself, Does it make sense to have worktree's as a
> separate struct or to have them stored in 'struct repository' in some
> way?

I'm not sure I have a good answer for this.  Looking at the libgit2
source, which has a repository struct for (I assume at least, I'm not
very familiar with the libgit2 source :)), they have a repository
struct for each worktree, with a flag specifying whether the struct is
for a worktree, or a "normal" git repository/the main worktree.

Obviously that doesn't mean we should do it the same way, but it
probably also doesn't hurt to look at prior art.

> Shouldn't a repository struct have a way to interact with all of
> its worktrees?

It could go either way I guess depending on how we want to design it.
If we want the repository struct to interact with all worktrees, it
seems to me like we would almost need some kind of 'struct
super_repository', which then contains a list of 'struct repository's
representing each worktree, probably including the main worktree.

But then again in normal operation we probably only want the
repository struct for the current repository we're working with, not
all of the worktrees.  I suspect it's only in some special cases where
we 

Q: rational for $XDG_CONFIG_HOME/git/config to be "non global" or just a bug?

[Yaroslav Halchenko]

Dear Git Gurus,

We [1] have got confused a bit about this recent addition of handling
$XDG_CONFIG_HOME/git/config -- is it --global or not? ;)

According to the man git-config (v 2.15.0 in debian)

   --global
   For writing options: write to global ~/.gitconfig file rather than
   the repository .git/config, write to $XDG_CONFIG_HOME/git/config
   file if this file exists and the ~/.gitconfig file doesn’t.

   For reading options: read only from global ~/.gitconfig and from
   $XDG_CONFIG_HOME/git/config rather than from all available files.

   See also the section called “FILES”.

suggesting that $XDG_CONFIG_HOME/git/config is a part of the "--global" config
space, which it is not, which is also later described in FILES:

   $(prefix)/etc/gitconfig
   System-wide configuration file.

   $XDG_CONFIG_HOME/git/config
   Second user-specific configuration file. If $XDG_CONFIG_HOME is not 
set or empty, $HOME/.config/git/config will be used. Any
   single-valued variable set in this file will be overwritten by 
whatever is in ~/.gitconfig. It is a good idea not to create this file if
   you sometimes use older versions of Git, as support for this file 
was added fairly recently.

   ~/.gitconfig
   User-specific configuration file. Also called "global" configuration 
file.

which

1. says that $XDG_CONFIG_HOME/git/config is the "Second user-specific ..."
   suggesting that it should be the one read AFTER the first user-specific...
   I guess that the first one is the ~/.gitconfig , but then why the first one
   overrides the settings of the second one ? ;)  (as described above in TFM and
   see below for an example)

2. why $XDG_CONFIG_HOME/git/config is not a part of the "global" configuration?

   I always assumed that "global" is ALL settings defined for a user,
   which are not specific to a repository.

   It is double-confusing since, as --global doc describes (and example
   below shows), git config --global --add modifies the
   $XDG_CONFIG_HOME/git/config if there is no ~/.gitconfig

   Actually the doc for --global for "reading" seems to be not correct,
   that the file is not consulted for --global (see below)

Example to show that TFM outlines precedence and --global correctly:

$> grep xdg .gitconfig .config/git/config  
.gitconfig:xdg-and-user = user
.config/git/config: xdg = xdg
.config/git/config: xdg-and-user = xdg
$> git config user.xdg ; git config user.xdg-and-user
xdg  
user
$> git config --global user.xdg# so outputs nothing
$> git config --global user.xdg-and-user
user

$> mv .gitconfig{,.aside}
$> git config --global --add user.new value 
$> cat .config/git/config 
[user]
 xdg = xdg
 xdg-and-user = xdg
 new = value


So, is that simply a bug and $XDG_CONFIG_HOME/git/config should be
consulted for --global reading and doc should be adjusted to
state that it is a part of "global" config in FILES description?
Or it shouldn't be --global (then writing should be fixed, and
documentation adjusted to exclude it from --global)
Or am I just confused? ;)

thanks in advance for the clarification!

[1]  https://github.com/datalad/datalad/pull/2019#issuecomment-350757960
-- 
Yaroslav O. Halchenko
Center for Open Neuroscience http://centerforopenneuroscience.org
Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755
Phone: +1 (603) 646-9834   Fax: +1 (603) 646-1419
WWW:   http://www.linkedin.com/in/yarik

Re: [PATCH] send-email: extract email-parsing code into a subroutine

[Matthieu Moy]

"PAYRE NATHAN p1508475"  wrote:

> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -685,7 +685,7 @@ Lines beginning in "GIT:" will be removed.
>  Consider including an overall diffstat or table of contents
>  for the patch you are writing.
>  
> -Clear the body content if you don't wish to send a summary.
> +Clear the body content if you dont wish to send a summary.

This is not part of your patch. Use "git add -p" to specify
exactly which hunks should go into the patch and don't let this
kind of change end up in the version you send.

> + my %parsed_email;
> + $parsed_email{'body'} = '';
> + while (my $line = <$c>) {
> + next if $line =~ m/^GIT:/;
> + parse_header_line($line, \%parsed_email);
> + if ($line =~ /^$/) {
> + $parsed_email{'body'} = filter_body($c);
>   }
> - print $c2 $_;

I didn't notice this at first, but you're modifying the behavior here:
the old code used to print to $c2 anything that didn't match any of
the if/else if branches.

To keep this behavior, you need to keep all these extra headers in
$parsed_email (you do, in this version) and print them after taking
care of all the known headers (AFAICT, you don't).

>   }
> - close $c;
> - close $c2;

You'll still need $c2, but you don't need $c anymore, so I'd keep the
"close $c" here. OTOH, $c2 is not needed before this point (actually a
bit later), so it would make sense to move the "open" down a little.
This would materialize the "read input, then write output" scheme (as
opposed to "write output while reading input" in the previous code).
It's not a new issue in your patch, but giving variables meaningful
names (i.e. not $c and $c2) would help, too.

> + if ($parsed_email{'mime-version'}) {
> + print $c2 "MIME-Version: $parsed_email{'mime-version'}\n",
> + "Content-Type: 
> $parsed_email{'content-type'};\n",
> + "Content-Transfer-Encoding: 
> $parsed_email{'content-transfer-encoding'}\n";
> + }
> +
> + if ($parsed_email{'content-type'}) {
> + print $c2 "MIME-Version: 1.0\n",
> +  "Content-Type: $parsed_email{'content-type'};\n",
> +  "Content-Transfer-Encoding: 8bit\n";

This "if ($parsed_email{'content-type'})" does not correspond to
anything in the old code, and ...

> + } elsif (file_has_nonascii($compose_filename)) {
> +my $content_type = ($parsed_email{'content-type'} or
> +"text/plain; charset=$compose_encoding");

Here, your're dealing explicitly with $parsed_email{'content-type'} !=
false (you're in the 'else' branch where it can only be false).

I think you just meant to drop the "if
($parsed_email{'content-type'})" part, and plug the "elseif" directly
after the "if ($parsed_email{'mime-version'})". That's what I
suggested in my earlier email.

> +my $content_type =3D ($parsed_email{'content-type'} or
> +"text/plain; charset=3D$compose_encoding");
> +print $c2 "MIME-Version: 1.0\n",
> +  "Content-Type: $content_type\n",
> +  "Content-Transfer-Encoding: 8bit\n";
> +}

This part is indented with spaces, please use tabs.

-- 
Matthieu Moy
https://matthieu-moy.fr/

Re: [PATCH 3/3] travis: run tests with GIT_TEST_SPLIT_INDEX

[SZEDER Gábor]

> Make sure that split index doesn't get broken, by running it on travis
> CI.
> 
> Run the test suite with split index enabled in linux 64 bit mode, and
> leave split index turned off in 32-bit mode.

This doesn't accurately describe what the patch does.
Travis CI runs three 64 bit Linux build jobs for us: one compiled with
Clang, one with GCC, and one with GETTEXT_POISON enabled.  This patch
enables split index only in the latter build job, but not in the Clang
and GCC build jobs.

>  The laternative would be
> to add an extra target in the matrix, enabling split index mode, but
> that would only use additional cycles on travis and would not bring that
> much benefit, as we are still running the test suite in the "vanilla"
> version in the 32-bit mode.
> 
> Signed-off-by: Thomas Gummerer 
> ---
>  .travis.yml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/.travis.yml b/.travis.yml
> index 281f101f31..c83c766dee 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -39,7 +39,7 @@ env:
>  
>  matrix:
>include:
> -- env: GETTEXT_POISON=YesPlease
> +- env: GETTEXT_POISON=YesPlease GIT_TEST_SPLIT_INDEX=YesPlease
>os: linux
>compiler:
>addons:
> -- 
> 2.15.1.504.g5279b80103

Re: [PATCH v1] convert: add support for 'encoding' attribute

[Johannes Sixt]

Am 11.12.2017 um 16:50 schrieb lars.schnei...@autodesk.com:

From: Lars Schneider 

Git and its tools (e.g. git diff) expect all text files in UTF-8
encoding. Git will happily accept content in all other encodings, too,
but it might not be able to process the text (e.g. viewing diffs or
changing line endings).

Add an attribute to tell Git what encoding the user has defined for a
given file. If the content is added to the index, then Git converts the
content to a canonical UTF-8 representation. On checkout Git will
reverse the conversion.

Reviewed-by: Patrick Lühne 
Signed-off-by: Lars Schneider 
---

Hi,

here is a WIP patch to add text encoding support for files encoded with
something other than UTF-8 [RFC].

The 'encoding' attribute is already used to view blobs in gitk. That
could be a problem as the content is stored in Git with the defined
encoding. This patch would interpret the content as UTF-8 encoded and


This will be a major drawback for me because my code base stores text 
files that are not UTF-8 encoded. And I do use the existing 'encoding' 
attribute to view the text in git-gui and gitk. Repurposing this 
attribute name is not an option, IMO.


it would try to reencode it to the defined encoding on checkout > Plus, many repos define the attribute very broad (e.g. "* 

encoding=cp1251").

These folks would see errors like these with my patch:
 error: failed to encode 'foo.bar' from utf-8 to cp1251


-- Hannes

Re: [PATCH Outreachy 1/2] format: create pretty.h file

[Junio C Hamano]

Оля Тележная   writes:

> Is it true that I need to fix only one commit message? (a typo
> s/futher/further/)
>
> Do you have any other advises what do I need to change?

I thought I mentioned that adding #include to all the current users
of "commit.h" is way too noisy.  I may have pointed out other issues
as well, but I offhand do not remember ;-)

Re: [PATCH v2 4/4] t/Makefile: introduce TEST_SHELL_PATH

[Junio C Hamano]

Jeff King  writes:

> I'm not sure that's true. Look at what already goes into
> GIT-BUILD-OPTIONS: TEST_OUTPUT_DIRECTORY, GIT_TEST_CMP, GIT_PERF_*, etc.
>
> Interestingly, many of those do something like this in the Makefile:
>
>   ifdef GIT_TEST_CMP
>   @echo GIT_TEST_OPTS=... >>$@+
>   endif
>
> which seems utterly confusing to me. Because it means that if you build
> with those options set, then they will override anything in the
> environment. But if you don't, then you _may_ override them in the
> environment. In other words:
>
>   make
>   cd t
>   GIT_TEST_CMP=foo ./t-*
>
> will respect that variable. But:
>
>   make GIT_TEST_CMP=foo
>   cd t
>   GIT_TEST_CMP=bar ./t-*
>
> will not. Which seems weird.  But I guess we could follow that pattern
> with TEST_SHELL_PATH.

Or perhaps we can start setting a better example with the new
variable, and migrate those weird existing ones over to the new way
of not forbidding run-time overriding?

There is a long outstanding NEEDSWORK comment in help.c that wonders
if we want to embed contents from GIT-BUILD-OPTIONS in the resulting
binary, and the distinction Dscho brought up between "build" and
"test" phases would start to matter even more once we go in that
direction.

Re: submodule modify/delete wrong message

[David Turner]

On Mon, 2017-12-11 at 12:27 -0800, Stefan Beller wrote:
> On Mon, Dec 4, 2017 at 1:39 PM, David Turner 
> wrote:
> > When merging with a submodule modify/delete conflict (i.e. I've
> > deleted
> > the submodule, and I'm merging in a branch that modified it), git
> > lies
> > about what it is doing:
> > 
> > "CONFLICT (modify/delete): submodule deleted in HEAD and modified
> > in
> > submodules.
> 
> Up to here the error message sounds correct, still?

Yep!

> > Version submodules of submodule left in tree at
> > submodule~submodules.
> > Automatic merge failed; fix conflicts and then commit the result."
> 
> This sounds as if the code assumed to handle only files.

This assumption is unfortunately very common -- I just filed a PR to
fix an instance of this in libgit2.

> > In fact, the working tree does not contain anything named
> > 'submodule~submodules'.
> > 
> > In addition, I would ordinarily resolve a conflict like this by
> > using
> > 'git rm'. Here, this gives a warning:
> > 
> > $ git rm submodule
> > submodule: needs merge
> 
> (Regarding submodule merges in general:)
> 
> Uh. We cannot add merge markers to a submodule or such.
> More importantly we'd have to ask the question if the merge conflict
> is on the superproject level (Choose one of the commits of the
> submodule)
> or on the submodule level (perform a merge in the submodule between
> the
> two commits) or some hybrid approach thereof.

Yeah, that's a tricky thing in general.  In this case, tho, the
submodule is being removed, so git rm should work.

> > rm 'submodule'
> > warning: Could not find section in .gitmodules where path=submodule
> 
> The deletion of the submodule removed the .gitmodules entry, and the
> merge of the .gitmodules file presumably went fine. :/

Indeed.

> I assume we need a special merge driver for the .gitmodules file to
> keep
> the submodule around when it is in at least one side.
> 
> > Git's behavior here is significantly better than liggit2's (which
> > tries
> > to check out 'submodule' as if it were a blob, and fails to do so),
> > but
> > it's still confusing.
> > 
> > It's not clear to me what the correct behavior is here.  Maybe it's
> > sufficient to just fix the message?
> 
> I think the first step is to fix the message to reflect reality.
> 
> As alluded to above, I don't know what the correct merge
> behavior is (and where to put 'conflict markers').

The only case we need 'conflict markers' is in the
{add,modify}/{add,modify} case (with different versions on each side). 
The delete/* and */delete case can be handled more simply.  We might
not want to do this until we can handle all cases -- but personally, I
think a half-solution is better than a non-solution.

Re: [PATCH 1/3] repository: fix repo_read_index with submodules

[Thomas Gummerer]

On 12/11, Brandon Williams wrote:
> On 12/10, Thomas Gummerer wrote:
> > repo_read_index calls read_index_from, which takes an path argument for
> > the location of the index file.  For the split index however it relies
> > on the current working directory to construct the path using git_path.
> > 
> > repo_read_index calls read_index_from with the full path for the index
> > file, however it doesn't change the cwd, so when split index mode is
> > turned on, read_index_from can't find the file for the split index.
> > 
> > For example t3007-ls-files-recurse-submodules.sh was broken with
> > GIT_TEST_SPLIT_INDEX set in 188dce131f ("ls-files: use repository
> > object", 2017-06-22), and t7814-grep-recurse-submodules.sh was also
> > broken in a similar manner, probably by introducing struct repository
> > there, although I didn't track down the exact commit for that.
> > 
> > Fix this by introducing a new read_index_for_repo function, which knows
> > about the correct paths for the submodules.
> > 
> > The alternative would have been to make the callers pass in the base
> > path for the split index, however that ended up being more complicated,
> > and I think we want to converge towards using struct repository for
> > things like these anyway.
> 
> Thanks for catching this, I'm not a user of split index myself which is
> why I unfortunately overlooked this.  Definitely a good change.  I
> really only have one nit below.

Me neither, I just remember to run the split index tests every once in
a while, which is also why it's taken so long to catch this.
Hopefully after we fixed this we can get travis to run the tests,
which should help :)

> > 
> > Signed-off-by: Thomas Gummerer 
> > ---
> >  cache.h  |  1 +
> >  read-cache.c | 19 +--
> >  repository.c |  2 +-
> >  3 files changed, 19 insertions(+), 3 deletions(-)
> > 
> > diff --git a/cache.h b/cache.h
> > index cb5db7bf83..d42bea1ef7 100644
> > --- a/cache.h
> > +++ b/cache.h
> > @@ -614,6 +614,7 @@ extern int read_index_preload(struct index_state *, 
> > const struct pathspec *paths
> >  extern int do_read_index(struct index_state *istate, const char *path,
> >  int must_exist); /* for testting only! */
> >  extern int read_index_from(struct index_state *, const char *path);
> > +extern int read_index_for_repo(const struct repository *);
> >  extern int is_index_unborn(struct index_state *);
> >  extern int read_index_unmerged(struct index_state *);
> >  
> > diff --git a/read-cache.c b/read-cache.c
> > index 2eb81a66b9..4d5c4ad79b 100644
> > --- a/read-cache.c
> > +++ b/read-cache.c
> > @@ -20,6 +20,7 @@
> >  #include "split-index.h"
> >  #include "utf8.h"
> >  #include "fsmonitor.h"
> > +#include "repository.h"
> >  
> >  /* Mask for the name length in ce_flags in the on-disk index */
> >  
> > @@ -1871,7 +1872,8 @@ static void freshen_shared_index(char *base_sha1_hex, 
> > int warn)
> > free(shared_index);
> >  }
> >  
> > -int read_index_from(struct index_state *istate, const char *path)
> > +static int do_read_index_from(struct index_state *istate, const char *path,
> > + const struct repository *repo)
> >  {
> > struct split_index *split_index;
> > int ret;
> > @@ -1896,7 +1898,10 @@ int read_index_from(struct index_state *istate, 
> > const char *path)
> > split_index->base = xcalloc(1, sizeof(*split_index->base));
> >  
> > base_sha1_hex = sha1_to_hex(split_index->base_sha1);
> > -   base_path = git_path("sharedindex.%s", base_sha1_hex);
> > +   if (repo)
> > +   base_path = repo_git_path(repo, "sharedindex.%s", 
> > base_sha1_hex);
> > +   else
> > +   base_path = git_path("sharedindex.%s", base_sha1_hex);
> > ret = do_read_index(split_index->base, base_path, 1);
> > if (hashcmp(split_index->base_sha1, split_index->base->sha1))
> > die("broken index, expect %s in %s, got %s",
> > @@ -1909,6 +1914,16 @@ int read_index_from(struct index_state *istate, 
> > const char *path)
> > return ret;
> >  }
> >  
> > +int read_index_for_repo(const struct repository *repo)
> > +{
> > +   return do_read_index_from(repo->index, repo->index_file, repo);
> > +}
> > +
> > +int read_index_from(struct index_state *istate, const char *path)
> > +{
> > +   return do_read_index_from(istate, path, NULL);
> > +}
> 
> Instead of passing NULL and having to special case it in
> 'do_read_index_from()', how about we pass in 'the_repository'?

I think that makes sense the only way this function used to work with
split index turned on is if it's called from the main repository, so
just passing through 'the_repository' would have the function behave
the exact same way as before.

> > +
> >  int is_index_unborn(struct index_state *istate)
> >  {
> > return (!istate->cache_nr && !istate->timestamp.sec);
> > diff --git a/repository.c b/repository.c
> > index bb2fae5446..928b1f553d 100644
> > --- a/repository.c
> > +++ 

Re: submodule modify/delete wrong message

[Stefan Beller]

On Mon, Dec 4, 2017 at 1:39 PM, David Turner  wrote:
> When merging with a submodule modify/delete conflict (i.e. I've deleted
> the submodule, and I'm merging in a branch that modified it), git lies
> about what it is doing:
>
> "CONFLICT (modify/delete): submodule deleted in HEAD and modified in
> submodules.

Up to here the error message sounds correct, still?


> Version submodules of submodule left in tree at
> submodule~submodules.
> Automatic merge failed; fix conflicts and then commit the result."

This sounds as if the code assumed to handle only files.

> In fact, the working tree does not contain anything named
> 'submodule~submodules'.
>
> In addition, I would ordinarily resolve a conflict like this by using
> 'git rm'. Here, this gives a warning:
>
> $ git rm submodule
> submodule: needs merge

(Regarding submodule merges in general:)

Uh. We cannot add merge markers to a submodule or such.
More importantly we'd have to ask the question if the merge conflict
is on the superproject level (Choose one of the commits of the submodule)
or on the submodule level (perform a merge in the submodule between the
two commits) or some hybrid approach thereof.

> rm 'submodule'
> warning: Could not find section in .gitmodules where path=submodule

The deletion of the submodule removed the .gitmodules entry, and the
merge of the .gitmodules file presumably went fine. :/

I assume we need a special merge driver for the .gitmodules file to keep
the submodule around when it is in at least one side.

> Git's behavior here is significantly better than liggit2's (which tries
> to check out 'submodule' as if it were a blob, and fails to do so), but
> it's still confusing.
>
> It's not clear to me what the correct behavior is here.  Maybe it's
> sufficient to just fix the message?

I think the first step is to fix the message to reflect reality.

As alluded to above, I don't know what the correct merge
behavior is (and where to put 'conflict markers').

Stefan

[PATCH 0/1] diff-core blobfind

[Stefan Beller]

* added check for unmerged entries (!DIFF_PAIR_UNMERGED)
* added support to find submodules
* renamed the UI to `--find-object` instead of --blobfind.

diff to currently queued below.

Thanks,
Stefan

Stefan Beller (1):
  diffcore: add a filter to find a specific blob

 Documentation/diff-options.txt |  6 +
 Makefile   |  1 +
 builtin/log.c  |  2 +-
 diff.c | 21 -
 diff.h |  3 +++
 diffcore-oidfind.c | 42 ++
 diffcore.h |  1 +
 revision.c |  5 -
 t/t4064-diff-oidfind.sh| 51 ++
 9 files changed, 129 insertions(+), 3 deletions(-)
 create mode 100644 diffcore-oidfind.c
 create mode 100755 t/t4064-diff-oidfind.sh

-- 
2.15.1.424.g9478a66081-goog

diff --git c/Documentation/diff-options.txt w/Documentation/diff-options.txt
index 34d53b95f1..67a99e522b 100644
--- c/Documentation/diff-options.txt
+++ w/Documentation/diff-options.txt
@@ -501,9 +501,10 @@ information.
Treat the  given to `-S` as an extended POSIX regular
expression to match.
 
---blobfind=::
+--find-object=::
Restrict the output such that one side of the diff
-   matches the given blob-id.
+   matches the given object id. The object can be a blob,
+   or gitlink entry.
 
 endif::git-format-patch[]
 
diff --git c/Makefile w/Makefile
index fdfa8f38f6..fc2b136694 100644
--- c/Makefile
+++ w/Makefile
@@ -775,7 +775,7 @@ LIB_OBJS += date.o
 LIB_OBJS += decorate.o
 LIB_OBJS += diffcore-break.o
 LIB_OBJS += diffcore-delta.o
-LIB_OBJS += diffcore-blobfind.o
+LIB_OBJS += diffcore-oidfind.o
 LIB_OBJS += diffcore-order.o
 LIB_OBJS += diffcore-pickaxe.o
 LIB_OBJS += diffcore-rename.o
diff --git c/builtin/log.c w/builtin/log.c
index 7b91f61423..2ab7f338e6 100644
--- c/builtin/log.c
+++ w/builtin/log.c
@@ -181,7 +181,7 @@ static void cmd_log_init_finish(int argc, const char 
**argv, const char *prefix,
init_display_notes(>notes_opt);
 
if (rev->diffopt.pickaxe || rev->diffopt.filter ||
-   rev->diffopt.flags.follow_renames || rev->diffopt.blobfind)
+   rev->diffopt.flags.follow_renames || rev->diffopt.oidfind)
rev->always_show_header = 0;
 
if (source)
diff --git c/diff.c w/diff.c
index 8861f89ab1..cb35934634 100644
--- c/diff.c
+++ w/diff.c
@@ -4082,7 +4082,7 @@ void diff_setup(struct diff_options *options)
options->interhunkcontext = diff_interhunk_context_default;
options->ws_error_highlight = ws_error_highlight_default;
options->flags.rename_empty = 1;
-   options->blobfind = NULL;
+   options->oidfind = NULL;
 
/* pathchange left =NULL by default */
options->change = diff_change;
@@ -4488,16 +4488,17 @@ static int parse_ws_error_highlight_opt(struct 
diff_options *opt, const char *ar
return 1;
 }
 
-static int parse_blobfind_opt(struct diff_options *opt, const char *arg)
+static int parse_oidfind_opt(struct diff_options *opt, const char *arg)
 {
struct object_id oid;
 
-   if (get_oid_blob(arg, ) || sha1_object_info(oid.hash, NULL) != 
OBJ_BLOB)
-   return error("object '%s' is not a blob", arg);
+   /* We don't even need to have the object, any oid works. */
+   if (get_oid_blob(arg, ))
+   return error("unable to resolve '%s'", arg);
 
-   if (!opt->blobfind)
-   opt->blobfind = xcalloc(1, sizeof(*opt->blobfind));
-   oidset_insert(opt->blobfind, );
+   if (!opt->oidfind)
+   opt->oidfind = xcalloc(1, sizeof(*opt->oidfind));
+   oidset_insert(opt->oidfind, );
return 1;
 }
 
@@ -4750,8 +4751,8 @@ int diff_opt_parse(struct diff_options *options,
else if ((argcount = short_opt('O', av, ))) {
options->orderfile = prefix_filename(prefix, optarg);
return argcount;
-   } else if (skip_prefix(arg, "--blobfind=", ))
-   return parse_blobfind_opt(options, arg);
+   } else if (skip_prefix(arg, "--find-object=", ))
+   return parse_oidfind_opt(options, arg);
else if ((argcount = parse_long_opt("diff-filter", av, ))) {
int offending = parse_diff_filter_opt(optarg, options);
if (offending)
@@ -5786,8 +5787,8 @@ void diffcore_std(struct diff_options *options)
if (!options->found_follow) {
/* See try_to_follow_renames() in tree-diff.c */
 
-   if (options->blobfind)
-   diffcore_blobfind(options);
+   if (options->oidfind)
+   diffcore_oidfind(options);
if (options->break_opt != -1)
diffcore_break(options->break_opt);
if (options->detect_rename)
diff --git c/diff.h w/diff.h
index 9178e498fa..d2badb29a1 100644
--- c/diff.h
+++ w/diff.h
@@ 

[PATCH 1/1] diffcore: add a filter to find a specific blob

[Stefan Beller]

Sometimes users are given a hash of an object and they want to
identify it further (ex.: Use verify-pack to find the largest blobs,
but what are these? or [1])

One might be tempted to extend git-describe to also work with blobs,
such that `git describe ` gives a description as
':'.  This was implemented at [2]; as seen by the sheer
number of responses (>110), it turns out this is tricky to get right.
The hard part to get right is picking the correct 'commit-ish' as that
could be the commit that (re-)introduced the blob or the blob that
removed the blob; the blob could exist in different branches.

Junio hinted at a different approach of solving this problem, which this
patch implements. Teach the diff machinery another flag for restricting
the information to what is shown. For example:

  $ ./git log --oneline --blobfind=v2.0.0:Makefile
  b2feb64309 Revert the whole "ask curl-config" topic for now
  47fbfded53 i18n: only extract comments marked with "TRANSLATORS:"

we observe that the Makefile as shipped with 2.0 was introduced in
v1.9.2-471-g47fbfded53 and replaced in v2.0.0-rc1-5-gb2feb64309 by
a different blob.

[1] https://stackoverflow.com/questions/223678/which-commit-has-this-blob
[2] https://public-inbox.org/git/20171028004419.10139-1-sbel...@google.com/

Signed-off-by: Stefan Beller 
---
 Documentation/diff-options.txt |  6 +
 Makefile   |  1 +
 builtin/log.c  |  2 +-
 diff.c | 21 -
 diff.h |  3 +++
 diffcore-oidfind.c | 42 ++
 diffcore.h |  1 +
 revision.c |  5 -
 t/t4064-diff-oidfind.sh| 51 ++
 9 files changed, 129 insertions(+), 3 deletions(-)
 create mode 100644 diffcore-oidfind.c
 create mode 100755 t/t4064-diff-oidfind.sh

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index dd0dba5b1d..67a99e522b 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -500,6 +500,12 @@ information.
 --pickaxe-regex::
Treat the  given to `-S` as an extended POSIX regular
expression to match.
+
+--find-object=::
+   Restrict the output such that one side of the diff
+   matches the given object id. The object can be a blob,
+   or gitlink entry.
+
 endif::git-format-patch[]
 
 -O::
diff --git a/Makefile b/Makefile
index ee9d5eb11e..fc2b136694 100644
--- a/Makefile
+++ b/Makefile
@@ -775,6 +775,7 @@ LIB_OBJS += date.o
 LIB_OBJS += decorate.o
 LIB_OBJS += diffcore-break.o
 LIB_OBJS += diffcore-delta.o
+LIB_OBJS += diffcore-oidfind.o
 LIB_OBJS += diffcore-order.o
 LIB_OBJS += diffcore-pickaxe.o
 LIB_OBJS += diffcore-rename.o
diff --git a/builtin/log.c b/builtin/log.c
index 6c1fa896ad..2ab7f338e6 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -181,7 +181,7 @@ static void cmd_log_init_finish(int argc, const char 
**argv, const char *prefix,
init_display_notes(>notes_opt);
 
if (rev->diffopt.pickaxe || rev->diffopt.filter ||
-   rev->diffopt.flags.follow_renames)
+   rev->diffopt.flags.follow_renames || rev->diffopt.oidfind)
rev->always_show_header = 0;
 
if (source)
diff --git a/diff.c b/diff.c
index 0763e89263..cb35934634 100644
--- a/diff.c
+++ b/diff.c
@@ -4082,6 +4082,7 @@ void diff_setup(struct diff_options *options)
options->interhunkcontext = diff_interhunk_context_default;
options->ws_error_highlight = ws_error_highlight_default;
options->flags.rename_empty = 1;
+   options->oidfind = NULL;
 
/* pathchange left =NULL by default */
options->change = diff_change;
@@ -4487,6 +4488,20 @@ static int parse_ws_error_highlight_opt(struct 
diff_options *opt, const char *ar
return 1;
 }
 
+static int parse_oidfind_opt(struct diff_options *opt, const char *arg)
+{
+   struct object_id oid;
+
+   /* We don't even need to have the object, any oid works. */
+   if (get_oid_blob(arg, ))
+   return error("unable to resolve '%s'", arg);
+
+   if (!opt->oidfind)
+   opt->oidfind = xcalloc(1, sizeof(*opt->oidfind));
+   oidset_insert(opt->oidfind, );
+   return 1;
+}
+
 int diff_opt_parse(struct diff_options *options,
   const char **av, int ac, const char *prefix)
 {
@@ -4736,7 +4751,8 @@ int diff_opt_parse(struct diff_options *options,
else if ((argcount = short_opt('O', av, ))) {
options->orderfile = prefix_filename(prefix, optarg);
return argcount;
-   }
+   } else if (skip_prefix(arg, "--find-object=", ))
+   return parse_oidfind_opt(options, arg);
else if ((argcount = parse_long_opt("diff-filter", av, ))) {
int offending = parse_diff_filter_opt(optarg, options);
if (offending)
@@ -5770,6 +5786,9 @@ 

Re: Shared clone from worktree directory

[Eric Sunshine]

On Mon, Dec 11, 2017 at 12:02 PM, Marc-André Lureau
 wrote:
> For better, or worse, I encountered a script doing a git clone
> --shared from the working directory. However, if clone --shared is run
> from a worktree, it fails with cryptic errors.
>
> elmarco@boraha:/tmp/test/wt (wt)$ git worktree list
> /tmp/test 4ae16a0 [master]
> /tmp/test/wt  4ae16a0 [wt]
> elmarco@boraha:/tmp/test/wt (wt)$ git clone --shared  . clone-dir
> Cloning into 'clone-dir'...
> done.
> error: object directory /tmp/test/.git/worktrees/wt/objects does not
> exist; check .git/objects/info/alternates.
> fatal: update_ref failed for ref 'HEAD': cannot update ref
> 'refs/heads/wt': trying to write ref 'refs/heads/wt' with nonexistent
> object 4ae16a066ee088d40dbefeaaae7b5578d68b4b51
> fatal: The remote end hung up unexpectedly
>
> Is this a bug? If not, a nicer error message would be welcome, as well
> as man page note.

Looks like a simple oversight in the 'worktree' implementation. I
worked up a patch to fix it, which I'll try to send out later today.

Re: [PATCH] git-send-email: fix get_maintainer.pl regression

[Ævar Arnfjörð Bjarmason]

On Mon, Dec 11, 2017 at 6:26 PM, Thomas Adam  wrote:
> On Mon, Dec 11, 2017 at 05:13:53PM +, Alex Bennée wrote:
>> So have we come to a consensus about the best solution here?
>>
>> I'm perfectly happy to send a reversion patch because to be honest
>> hacking on a bunch of perl to handle special mail cases is not my idea
>> of fun spare time hacking ;-)
>>
>> I guess the full solution is to make Mail::Address a hard dependency?
>
> This is what I was suggesting, and then as a follow-up, addressing the point
> that there's a bunch of require() hacks to also get around needing
> hard-dependencies.

I don't know what the right move is here, but just saying that this
could also be built on top of my "Git::Error" wrapper which I added in
"Makefile: replace perl/Makefile.PL with simple make rules" which is
currently cooking.

I.e. we'd just ship a copy of Email::Valid and Mail::Address in
perl/Git/FromCPAN/, use a wrapper to load them, and then we wouldn't
need to if/else this at the code level, just always use the module,
and it would work even on core perl.

Re: [PATCH v7 00/16] Parial clone part 3: clone, fetch, fetch-pack, upload-pack, and tests

[Jonathan Tan]

On Fri, 8 Dec 2017 14:30:10 -0800
Brandon Williams  wrote:

> I just finished reading through parts 1-3.  Overall I like the series.
> There are a few point's that I'm not a big fan of but i wasn't able to
> come up with a better alternative.  One of these being the need for a
> global variable to tell the fetch-object logic to not go to the server
> to try and fetch a missing object.

I didn't really like that approach too but I went with that because,
like you, I couldn't come up with a better one. The main issue is that
too many functions (e.g. parse_commit() in commit.c) indirectly read
objects, and I couldn't find a better way to control them all. Ideally,
we should have a "struct object_store" (or maybe "struct repository"
could do this too) on which we can set "fetch_if_missing", and have all
object-reading functions take a pointer to this struct. Or completely
separate the object-reading and object-parsing code (e.g. commit.c
should not be able to read objects at all). Or both.

Any of these would be major undertakings, though, and there are good
reasons for why the same function does the reading and parsing (for
example, parse_commit() does not perform any reading if the object has
been already parsed).

> One other thing i noticed was it looks like when you discover that you
> are missing a blob you you'll try to fault it in from the server without
> first checking its an object the server would even have.  Shouldn't you
> first do a check to verify that the object in question is a promised
> object before you go out to contact the server to request it?  You may
> have already ruled this out for some reason I'm not aware of (maybe its
> too costly to compute?).

It is quite costly to compute - in the worst case, we would need to read
every object in every promisor packfile of one or more certain types
(e.g. if we know that we're fetching a blob, we need to read every tree)
to find out if the object we want is a promisor object.

Such a check would be better at surfacing mistakes (e.g. the user giving
the wrong SHA-1) early, but beyond that, I don't think that having the
check is very important. Consider these two very common situations:

 (1) Fetching a single branch by its tip's SHA-1. A naive implementation
 will first check if we have that SHA-1, which triggers the dynamic
 fetch (since it is an object read), and assuming success, notice
 that we indeed have that tip, and not fetch anything else. The
 check you describe will avoid this situation.
 (2) Dynamically fetching a missing blob by its SHA-1. A naive
 implementation will first check if we have that SHA-1, which
 triggers the dynamic fetch, and that fetch will first check if we
 have that SHA-1, and so on (thus, an infinite loop). The check you
 describe will not avoid that situation.

The check solves (1), but we still need a solution to (2) - I used
"fetch_if_missing", as discussed in your previous question and my answer
to that. A solution to (2) is usually also a solution to (1), so the
check wouldn't help much here.

Re: [PATCH 2/3] prune: fix pruning with multiple worktrees and split index

[Brandon Williams]

On 12/10, Thomas Gummerer wrote:
> be489d02d2 ("revision.c: --indexed-objects add objects from all
> worktrees", 2017-08-23) made sure that pruning takes objects from all
> worktrees into account.
> 
> It did that by reading the index of every worktree and adding the
> necessary index objects to the set of pending objects.  The index is
> read by read_index_from.  As mentioned in the previous commit,
> read_index_from depends on the CWD for the location of the split index,
> and add_index_objects_to_pending doesn't set that before using
> read_index_from.
> 
> Instead of using read_index_from, use repo_read_index, which is aware of
> the proper paths for the worktree.
> 
> This fixes t5304-prune when ran with GIT_TEST_SPLIT_INDEX set.
> 

I'm on the fence about this change.  I understand that this will ensure
that the proper objects aren't pruned when using a split index in the
presence of worktrees but I think the solution needs to be thought
through a bit more.

My big concern right now is the interaction of 'struct worktree's and
'struct repository'.  I'll try to highlight my concerns below.

> Signed-off-by: Thomas Gummerer 
> ---
> 
> This also fixes t7009 when ran with GIT_TEST_SPLIT_INDEX.  I'm not
> quite sure why it is fixed by this.  Either way I tracked the failure
> down to f767178a5a ("Merge branch 'jk/no-null-sha1-in-cache-tree'",
> 2017-05-16).  Maybe Peff has an idea why this fixes that test?
> 
>  repository.c | 11 +++
>  repository.h |  2 ++
>  revision.c   | 13 -
>  3 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/repository.c b/repository.c
> index 928b1f553d..3c9bfbd1b8 100644
> --- a/repository.c
> +++ b/repository.c
> @@ -2,6 +2,7 @@
>  #include "repository.h"
>  #include "config.h"
>  #include "submodule-config.h"
> +#include "worktree.h"
>  
>  /* The main repository */
>  static struct repository the_repo = {
> @@ -146,6 +147,16 @@ int repo_init(struct repository *repo, const char 
> *gitdir, const char *worktree)
>   return -1;
>  }
>  
> +/*
> + * Initialize 'repo' based on the provided worktree
> + * Return 0 upon success and a non-zero value upon failure.
> + */
> +int repo_worktree_init(struct repository *repo, struct worktree *worktree)
> +{
> + return repo_init(repo, get_worktree_git_dir(worktree),
> +  worktree->path);
> +}

My first concern is the use of 'get_worktree_git_dir()'.  Under the hood
it calls 'get_git_dir()', 'get_git_common_dir()', and
'git_common_path()' which rely on global state as stored in
'the_repository'.  So how does one initialize a repository struct (using
this initializer) using a worktree from a repository other than the
global 'the_repository' struct?  I'm not sure I have an answer right
now, but its an issue that needs to be thought through before we head
down this road.

Just thinking to myself, Does it make sense to have worktree's as a
separate struct or to have them stored in 'struct repository' in some
way?  Shouldn't a repository struct have a way to interact with all of
its worktrees?  How would initializing a repository struct for every
worktree work once we migrate the object store to be stored in 'struct
repoisotry'?  Shouldn't every worktree share the same object store
in-memory like they do on-disk?

> +
>  /*
>   * Initialize 'submodule' as the submodule given by 'path' in parent 
> repository
>   * 'superproject'.
> diff --git a/repository.h b/repository.h
> index 7f5e24a0a2..2adeb05bf4 100644
> --- a/repository.h
> +++ b/repository.h
> @@ -4,6 +4,7 @@
>  struct config_set;
>  struct index_state;
>  struct submodule_cache;
> +struct worktree;
>  
>  struct repository {
>   /* Environment */
> @@ -87,6 +88,7 @@ extern struct repository *the_repository;
>  extern void repo_set_gitdir(struct repository *repo, const char *path);
>  extern void repo_set_worktree(struct repository *repo, const char *path);
>  extern int repo_init(struct repository *repo, const char *gitdir, const char 
> *worktree);
> +extern int repo_worktree_init(struct repository *repo, struct worktree 
> *worktree);
>  extern int repo_submodule_init(struct repository *submodule,
>  struct repository *superproject,
>  const char *path);
> diff --git a/revision.c b/revision.c
> index e2e691dd5a..9d8d9b96d1 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -22,6 +22,7 @@
>  #include "packfile.h"
>  #include "worktree.h"
>  #include "argv-array.h"
> +#include "repository.h"
>  
>  volatile show_early_output_fn_t show_early_output;
>  
> @@ -1346,15 +1347,17 @@ void add_index_objects_to_pending(struct rev_info 
> *revs, unsigned int flags)
>   worktrees = get_worktrees(0);
>   for (p = worktrees; *p; p++) {
>   struct worktree *wt = *p;
> - struct index_state istate = { NULL };
> + struct repository *repo;
>  
> + repo = xmalloc(sizeof(struct repository));

This was 

Re: [PATCH 1/3] repository: fix repo_read_index with submodules

[Brandon Williams]

On 12/10, Thomas Gummerer wrote:
> repo_read_index calls read_index_from, which takes an path argument for
> the location of the index file.  For the split index however it relies
> on the current working directory to construct the path using git_path.
> 
> repo_read_index calls read_index_from with the full path for the index
> file, however it doesn't change the cwd, so when split index mode is
> turned on, read_index_from can't find the file for the split index.
> 
> For example t3007-ls-files-recurse-submodules.sh was broken with
> GIT_TEST_SPLIT_INDEX set in 188dce131f ("ls-files: use repository
> object", 2017-06-22), and t7814-grep-recurse-submodules.sh was also
> broken in a similar manner, probably by introducing struct repository
> there, although I didn't track down the exact commit for that.
> 
> Fix this by introducing a new read_index_for_repo function, which knows
> about the correct paths for the submodules.
> 
> The alternative would have been to make the callers pass in the base
> path for the split index, however that ended up being more complicated,
> and I think we want to converge towards using struct repository for
> things like these anyway.

Thanks for catching this, I'm not a user of split index myself which is
why I unfortunately overlooked this.  Definitely a good change.  I
really only have one nit below.

> 
> Signed-off-by: Thomas Gummerer 
> ---
>  cache.h  |  1 +
>  read-cache.c | 19 +--
>  repository.c |  2 +-
>  3 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/cache.h b/cache.h
> index cb5db7bf83..d42bea1ef7 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -614,6 +614,7 @@ extern int read_index_preload(struct index_state *, const 
> struct pathspec *paths
>  extern int do_read_index(struct index_state *istate, const char *path,
>int must_exist); /* for testting only! */
>  extern int read_index_from(struct index_state *, const char *path);
> +extern int read_index_for_repo(const struct repository *);
>  extern int is_index_unborn(struct index_state *);
>  extern int read_index_unmerged(struct index_state *);
>  
> diff --git a/read-cache.c b/read-cache.c
> index 2eb81a66b9..4d5c4ad79b 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -20,6 +20,7 @@
>  #include "split-index.h"
>  #include "utf8.h"
>  #include "fsmonitor.h"
> +#include "repository.h"
>  
>  /* Mask for the name length in ce_flags in the on-disk index */
>  
> @@ -1871,7 +1872,8 @@ static void freshen_shared_index(char *base_sha1_hex, 
> int warn)
>   free(shared_index);
>  }
>  
> -int read_index_from(struct index_state *istate, const char *path)
> +static int do_read_index_from(struct index_state *istate, const char *path,
> +   const struct repository *repo)
>  {
>   struct split_index *split_index;
>   int ret;
> @@ -1896,7 +1898,10 @@ int read_index_from(struct index_state *istate, const 
> char *path)
>   split_index->base = xcalloc(1, sizeof(*split_index->base));
>  
>   base_sha1_hex = sha1_to_hex(split_index->base_sha1);
> - base_path = git_path("sharedindex.%s", base_sha1_hex);
> + if (repo)
> + base_path = repo_git_path(repo, "sharedindex.%s", 
> base_sha1_hex);
> + else
> + base_path = git_path("sharedindex.%s", base_sha1_hex);
>   ret = do_read_index(split_index->base, base_path, 1);
>   if (hashcmp(split_index->base_sha1, split_index->base->sha1))
>   die("broken index, expect %s in %s, got %s",
> @@ -1909,6 +1914,16 @@ int read_index_from(struct index_state *istate, const 
> char *path)
>   return ret;
>  }
>  
> +int read_index_for_repo(const struct repository *repo)
> +{
> + return do_read_index_from(repo->index, repo->index_file, repo);
> +}
> +
> +int read_index_from(struct index_state *istate, const char *path)
> +{
> + return do_read_index_from(istate, path, NULL);
> +}

Instead of passing NULL and having to special case it in
'do_read_index_from()', how about we pass in 'the_repository'?

> +
>  int is_index_unborn(struct index_state *istate)
>  {
>   return (!istate->cache_nr && !istate->timestamp.sec);
> diff --git a/repository.c b/repository.c
> index bb2fae5446..928b1f553d 100644
> --- a/repository.c
> +++ b/repository.c
> @@ -229,5 +229,5 @@ int repo_read_index(struct repository *repo)
>   if (!repo->index)
>   repo->index = xcalloc(1, sizeof(*repo->index));
>  
> - return read_index_from(repo->index, repo->index_file);
> + return read_index_for_repo(repo);
>  }
> -- 
> 2.15.1.504.g5279b80103
> 

-- 
Brandon Williams

Re: [PATCH v5 7/9] sequencer: load commit related config

[Phillip Wood]

On 11/12/17 14:13, Phillip Wood wrote:
> From: Phillip Wood 
> 
> Load default values for message cleanup, gpg signing of commits and
> basic diff configuration in preparation for committing without forking
> 'git commit'. Note that we interpret commit.cleanup=scissors to mean
> COMMIT_MSG_CLEANUP_SPACE to be consistent with 'git commit'.
> 
> The sequencer should probably have been calling
> git_diff_basic_config() before as it creates a patch when there are
> conflicts. The shell version uses 'diff-index' to create the patch so

s/diff-index/diff-tree/

Re: [PATCH v1] convert: add support for 'encoding' attribute

[Eric Sunshine]

On Mon, Dec 11, 2017 at 10:50 AM,   wrote:
> From: Lars Schneider 
>
> Git and its tools (e.g. git diff) expect all text files in UTF-8
> encoding. Git will happily accept content in all other encodings, too,
> but it might not be able to process the text (e.g. viewing diffs or
> changing line endings).
>
> Add an attribute to tell Git what encoding the user has defined for a
> given file. If the content is added to the index, then Git converts the
> content to a canonical UTF-8 representation. On checkout Git will
> reverse the conversion.
>
> Reviewed-by: Patrick Lühne 
> Signed-off-by: Lars Schneider 
> ---
> diff --git a/convert.c b/convert.c
> @@ -256,6 +257,149 @@ static int will_convert_lf_to_crlf(size_t len, struct 
> text_stat *stats,
> +static int encode_to_git(const char *path, const char *src, size_t src_len,
> +struct strbuf *buf, struct encoding *enc)
> +{
> +#ifndef NO_ICONV
> +   char *dst, *re_src;
> +   int dst_len, re_src_len;
> +
> +   /*
> +* No encoding is specified or there is nothing to encode.
> +* Tell the caller that the content was not modified.
> +*/
> +   if (!enc || (src && !src_len))
> +   return 0;
> +
> +   /*
> +* Looks like we got called from "would_convert_to_git()".
> +* This means Git wants to know if it would encode (= modify!)
> +* the content. Let's answer with "yes", since an encoding was
> +* specified.
> +*/
> +   if (!buf && !src)
> +   return 1;
> +
> +   if (enc->to_git == invalid_conversion) {
> +   enc->to_git = iconv_open(default_encoding, encoding->name);
> +   if (enc->to_git == invalid_conversion)
> +   warning(_("unsupported encoding %s"), encoding->name);
> +   }
> +
> +   if (enc->to_worktree == invalid_conversion)
> +   enc->to_worktree = iconv_open(encoding->name, 
> default_encoding);

Do you need to be calling iconv_close() somewhere on the result of the
iconv_open() calls? [Answering myself after reading the rest of the
patch: You're caching these opened 'iconv' descriptors, so you don't
plan on closing them.]

> + [...]
> +   /*
> +* Encode dst back to ensure no information is lost. This wastes
> +* a few cycles as most conversions are round trip conversion
> +* safe. However, content that has an invalid encoding might not
> +* match its original byte sequence after the UTF-8 conversion
> +* round trip. Let's play safe here and check the round trip
> +* conversion.
> +*/
> +   re_src = reencode_string_iconv(dst, dst_len, enc->to_worktree, 
> _src_len);
> +   if (!re_src || strcmp(src, re_src)) {

You're using strcmp() as opposed to memcmp() because you expect
're_src' will unconditionally be UTF-8-encoded, right?

> +   die(_("encoding '%s' from %s to %s and back is not the same"),
> +   path, enc->name, default_encoding);
> +   }
> +   free(re_src);
> +
> +   strbuf_attach(buf, dst, dst_len, dst_len + 1);
> +   return 1;
> +#else
> +   warning(_("cannot encode '%s' from %s to %s because "
> +   "your Git was not compiled with encoding support"),
> +   path, enc->name, default_encoding);
> +   return 0;
> +#endif
> +}

Re: "git describe" documentation and behavior mismatch

[Daniel Knittl-Frank]

Forget the above patch. I should compile my code after refactoring ...

Here is the fixed version.

-- >8 --

>From 8203bd0ad5baab7024ebff597c9f35a0250d09ff Mon Sep 17 00:00:00 2001
From: Daniel Knittl-Frank 
Date: Mon, 11 Dec 2017 19:24:54 +0100
Subject: [PATCH] Prepend "tags/" when describing tags with embedded name

Signed-off-by: Daniel Knittl-Frank 
---
 builtin/describe.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index e14e162ef6..9da6d85ea3 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -271,10 +271,13 @@ static void display_name(struct commit_name *n)
 n->name_checked = 1;
 }

-if (n->tag)
+if (n->tag) {
+if (all)
+printf("tags/");
 printf("%s", n->tag->tag);
-else
+} else {
 printf("%s", n->path);
+}
 }

 static void show_suffix(int depth, const struct object_id *oid)
-- 
2.15.GIT

-- 
typed with http://neo-layout.org

Re: "git describe" documentation and behavior mismatch

[Daniel Knittl-Frank]

On Sun, Dec 3, 2017 at 6:39 AM, Junio C Hamano  wrote:
> I suspect that "see if the name recorded in the tag object matches
> the name of the ref that stores the tag after refs/tags/" code *is*
> not just verifying what it claims to (which may be good) but also
> unintentionally affecting the output (i.e. "--all" promises that the
> prefix tags/ should be shown).  Perhaps the code needs to be fixed
> if that is the case.

What is the course of action then? I wrote up a really dumb 2-line
patch which simply checks if --all was specified and prepends the
output with "tags/".

Good enough? Should we instead update the documentation? Still not
sure, what the behavior _should_ be in the case of annotated tags with
embedded names.

-- >8 --

>From 7243d700aad280b11e647e04ade027c412dde54c Mon Sep 17 00:00:00 2001
From: Daniel Knittl-Frank 
Date: Mon, 11 Dec 2017 19:24:54 +0100
Subject: [PATCH] Prepend "tags/" when describing tags with embedded name

Signed-off-by: Daniel Knittl-Frank 
---
 builtin/describe.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/describe.c b/builtin/describe.c
index e14e162ef6..54aaf30562 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -272,6 +272,8 @@ static void display_name(struct commit_name *n)
 }

 if (n->tag)
+if (all)
+printf("tags/");
 printf("%s", n->tag->tag);
 else
 printf("%s", n->path);
-- 
2.15.GIT



-- 
typed with http://neo-layout.org

Re: [PATCH] decorate: clean up and document API

[Jonathan Tan]

On Fri, 8 Dec 2017 04:55:11 -0500
Jeff King  wrote:

> I have mixed feelings. On the one hand, compiling and running the code
> ensures that those things actually work. On the other hand, I expect you
> can make a much clearer example if instead of having running code, you
> show snippets of almost-code.
> 
> E.g.:
> 
>   struct decoration d = { NULL };
> 
>   add_decoration(, obj, "foo");
>   ...
>   str = lookup_decoration(obj);
> 
> pretty much gives the relevant overview, with very little boilerplate.
> Yes, it omits things like the return value of add_decoration(), but
> those sorts of details are probably better left to the function
> docstrings.

The part about iterating over all entries should probably also be shown
too. Besides that, I'm OK with having a simplified example in
documentation too, but I'll wait and see if others have any opinions
before making any changes.

> Other than that philosophical point, the documentation you added looks
> pretty good to me. Two possible improvements to the API we could do on
> top:
> 
>   1. Should there be a DECORATION_INIT macro (possibly taking the "name"
>  as an argument)? (Actually, the whole name thing seems like a
>  confusing and bad API design in the first place).

Agreed about the "name" thing. I'll add a DECORATION_INIT when I make
the next reroll, but I think that having it with no argument is best
(and instantiating "name" with NULL).

>   2. This is really just an oidmap to a void pointer. I wonder if we
>  ought to be wrapping that code (I think we still want some
>  interface so that the caller doesn't have to declare their own
>  structs).

It is slightly different from oidmap in that this uses "struct object *"
as a key whereas oidmap uses "struct object_id", meaning that a user of
"decorate" must already have objects allocated or be willing to allocate
them, whereas a user of "oidmap" doesn't.

Having said that, it is true that perhaps we have too many data
structures doing similar things.

Re: [PATCH] git-send-email: fix get_maintainer.pl regression

[Thomas Adam]

On Mon, Dec 11, 2017 at 05:13:53PM +, Alex Bennée wrote:
> So have we come to a consensus about the best solution here?
> 
> I'm perfectly happy to send a reversion patch because to be honest
> hacking on a bunch of perl to handle special mail cases is not my idea
> of fun spare time hacking ;-)
> 
> I guess the full solution is to make Mail::Address a hard dependency?

This is what I was suggesting, and then as a follow-up, addressing the point
that there's a bunch of require() hacks to also get around needing
hard-dependencies.

-- Thomas Adam

RE: Shared clone from worktree directory

[Randall S. Becker]

On December 11, 2017 12:02 PM, Marc-André Lureau wrote:
>For better, or worse, I encountered a script doing a git clone --shared from 
>the working directory. However, if clone --shared is run from a worktree, it 
>fails with cryptic errors.
>elmarco@boraha:/tmp/test/wt (wt)$ git worktree list
>/tmp/test 4ae16a0 [master]
>/tmp/test/wt  4ae16a0 [wt]
>elmarco@boraha:/tmp/test/wt (wt)$ git clone --shared  . clone-dir Cloning into 
>'clone-dir'...
>done.
>error: object directory /tmp/test/.git/worktrees/wt/objects does not exist; 
>check .git/objects/info/alternates.
>fatal: update_ref failed for ref 'HEAD': cannot update ref
>'refs/heads/wt': trying to write ref 'refs/heads/wt' with nonexistent object 
>4ae16a066ee088d40dbefeaaae7b5578d68b4b51
>fatal: The remote end hung up unexpectedly
>Is this a bug? If not, a nicer error message would be welcome, as well as man 
>page note.

"The add worktree documentation states:
Create  and checkout  into it. The new working directory is 
linked to the ***current repository***, sharing everything except working 
directory specific files such as HEAD, index, etc. - may also be specified as 
; it is synonymous with @{-1}."

So I'm going to assume that clone from a worktree is not supported. This sounds 
like a check is needed to prevent the operation from starting up in the first 
place, or changing the semantics to allow it. The error message, while cryptic, 
seems actually descriptive because the HEAD would not be available in a 
worktree as it is not propagated from the current repository.

If the idea is to support an add worktree from a worktree, I would suggest that 
a new branch go back to the main repository as normal, rather than being added 
to the worktree. I personally get a headache thinking about the prospect of 
having layers of worktrees.

Cheers,
Randall

-- Brief whoami: NonStop developer since approximately 
UNIX(421664400)/NonStop(2112884442) 
-- In my real life, I talk too much.

Re: [PATCH] git-send-email: fix get_maintainer.pl regression

[Alex Bennée]

Junio C Hamano  writes:

> Thomas Adam  writes:
>
>> Trying to come up with a reinvention of regexps for email addresses is asking
>> for trouble, not to mention a crappy rod for your own back.  Don't do that.
>> This is why people use Mail::Address.
>>
>> https://metacpan.org/pod/distribution/MailTools/lib/Mail/Address.pod
>
> Now we are coming back to cc907506 ("send-email: don't use
> Mail::Address, even if available", 2017-08-23).  It argues
>
> * Having this optional Mail::Address makes it tempting to anwser "please
>   install Mail::Address" to users instead of fixing our own code. We've
>   reached the stage where bugs in our parser should be fixed, not worked
>   around.
>
> but if it costs us maintaining our substitute that much, it seems to
> me that depending on Mail::Address is not just tempting but may be a
> sensible way forward.
>
> Was there any reason why Mail::Address was _inadequate_?  I know we
> had trouble with random garbage that are *not* addresses people put
> on the in-body CC: trailer in the past, but I do not recall if they
> are something Mail::Address would give worse result and we need our
> workaround (hence our own substitute), or Mail::Address would handle
> them just fine.

Ping?

So have we come to a consensus about the best solution here?

I'm perfectly happy to send a reversion patch because to be honest
hacking on a bunch of perl to handle special mail cases is not my idea
of fun spare time hacking ;-)

I guess the full solution is to make Mail::Address a hard dependency?

--
Alex Bennée

Shared clone from worktree directory

[Marc-André Lureau]

Hi,

For better, or worse, I encountered a script doing a git clone
--shared from the working directory. However, if clone --shared is run
from a worktree, it fails with cryptic errors.

Ex:
elmarco@boraha:/tmp/test/wt (wt)$ git worktree list
/tmp/test 4ae16a0 [master]
/tmp/test/wt  4ae16a0 [wt]
elmarco@boraha:/tmp/test/wt (wt)$ git clone --shared  . clone-dir
Cloning into 'clone-dir'...
done.
error: object directory /tmp/test/.git/worktrees/wt/objects does not
exist; check .git/objects/info/alternates.
fatal: update_ref failed for ref 'HEAD': cannot update ref
'refs/heads/wt': trying to write ref 'refs/heads/wt' with nonexistent
object 4ae16a066ee088d40dbefeaaae7b5578d68b4b51
fatal: The remote end hung up unexpectedly

Is this a bug? If not, a nicer error message would be welcome, as well
as man page note.

thanks

-- 
Marc-André Lureau

Hallo

[Frank Koswell]

United Nations Compensation Unit, in Verbindung mit der Weltbank
Unsere Ref: U.N.O / W.BO / 11.11.2017 / 1982/09/05.

Glückwunsch, Begünstigter,

Wir haben eng mit der INTERPOL, CIA, FBI und anderen ausländischen
internationalen Organisationen sowie Western Union und Money Gram
bezüglich aller Zahlungen, die Sie in der Vergangenheit geleistet
haben, zusammengearbeitet und wir haben die vollständigen Listen und
Beträge, die Sie bisher gemacht haben. Bis zu diesem Zeitpunkt haben
Sie Ihre Zahlung jedoch nicht erhalten. Wir müssen Sie darüber
informieren, dass der frühere Beamte, mit dem Sie verhandelt haben,
festgenommen wurde und bald vor Gericht gestellt und vor Gericht
gestellt werden würde.


Wir haben jetzt ein Treffen zum Aufhören, und wir sind vor 72 Stunden
in Verbindung mit dem Präsidenten der Weltbank zu einer logischen
Schlussfolgerung gekommen. Ihre E-Mail-Adresse wurde unter denen
aufgeführt, die noch keine Ausgleichszahlung erhalten haben. Die
Vereinten Nationen in Verbindung mit der Weltbank haben sich darauf
geeinigt, sie nur mit der Summe von 1.500.000,00 USD (eine Million
fünfhunderttausend US-Dollar) zu entschädigen.


Aus diesem Grund erhalten Sie Ihre Zahlung über eine zertifizierte ATM
MASTER CARD. Beachten Sie, dass Sie mit dieser Master Card Geld aus
jedem Teil der Welt ohne Störung oder Verspätung abheben können und
bitte ohne Angabe von Gründen, sollten Sie Ihre Kontoinformationen
offenlegen, da Ihre Kontoinformationen vor Erhalt Ihrer Kartenzahlung
nicht benötigt werden. Alles, was jetzt von Ihnen verlangt wird, ist,
unsere 100% Vertrauenspersonen durch den Namen von Frau Sarah Ngene zu
kontaktieren. Unten finden Sie ihre Kontaktinformationen:


Name: Frau Katie Higgins

katiehiggins...@gmail.com

Bitte stellen Sie sicher, dass Sie die Anweisungen und Anweisungen von
Frau Katie Higgins befolgen, damit Sie innerhalb von 72 Stunden Ihre
Kartenzahlung erhalten und Ihren Geheimcode direkt aus
Sicherheitsgründen erhalten haben. Wir entschuldigen uns im Namen der
Organisation der Vereinten Nationen für jede Verzögerung, die Sie in
der Vergangenheit beim Empfang Ihres Fonds erfahren haben könnten.
Herzlichen Glückwunsch, und ich freue mich darauf, von Ihnen zu hören,
sobald Sie Ihre Zahlung bestätigen und die Welt zu einem besseren Ort
machen.


Hochachtungsvoll,

Frank Koswell

Untergeneralsekretär für den Wirtschafts- und Sozialrat

[PATCH v1] convert: add support for 'encoding' attribute

[lars . schneider]

From: Lars Schneider 

Git and its tools (e.g. git diff) expect all text files in UTF-8
encoding. Git will happily accept content in all other encodings, too,
but it might not be able to process the text (e.g. viewing diffs or
changing line endings).

Add an attribute to tell Git what encoding the user has defined for a
given file. If the content is added to the index, then Git converts the
content to a canonical UTF-8 representation. On checkout Git will
reverse the conversion.

Reviewed-by: Patrick Lühne 
Signed-off-by: Lars Schneider 
---

Hi,

here is a WIP patch to add text encoding support for files encoded with
something other than UTF-8 [RFC].

The 'encoding' attribute is already used to view blobs in gitk. That
could be a problem as the content is stored in Git with the defined
encoding. This patch would interpret the content as UTF-8 encoded and
it would try to reencode it to the defined encoding on checkout.
Plus, many repos define the attribute very broad (e.g. "* encoding=cp1251").
These folks would see errors like these with my patch:
error: failed to encode 'foo.bar' from utf-8 to cp1251

A quick search on GitHub reveals 2,722 repositories that use the
'encoding' attribute [1]. Using the GitHub API [2], I identified the
following encodings in all publicly accessible repositories:

ANSI<-- garbage (ignore by my implementation)
cp1251
cp866
cp950
iso8859-1
koi8-r
shiftjis<-- garbage (ignore by my implementation)
UTF-8   <-- unnecessary (ignore by my implementation)
utf8<-- garbage (ignore by my implementation)

TODOs:
- The iconv docs mention that "errno == EINVAL" is harmless but
  we don't handle that case in utf8.c:reencode_string_iconv()
- Git does not compile with NO_ICONV=1 right now because of
  compat/precompose_utf8.c. I will send a patch to fix that.

Questions:
- Should I use warning() or error() in the patch?
  (currently I use the warning)
- Do you agree with the approach in general?

Thanks,
Lars


[RFC] http://public-inbox.org/git/bdb9b884-6d17-4be3-a83c-f67e2afa2...@gmail.com
  [1] 
https://github.com/search?p=1=encoding+filename%3Agitattributes=Code=%E2%9C%93
  [2] curl --user larsxschneider:secret -H 'Accept: 
application/vnd.github.v3.text-match+json' 
'https://api.github.com/search/code?q=encoding+in:file+filename:gitattributes' 
| jq -r '.items[].text_matches[].fragment' | sed 's/.*encoding=//' | sort | uniq
  [3] 
https://www.gnu.org/software/libc/manual/html_node/iconv-Examples.html#iconv-Examples




Notes:
Base Ref: master
Web-Diff: https://github.com/larsxschneider/git/commit/afc9e88a4d
Checkout: git fetch https://github.com/larsxschneider/git encoding-v1 && 
git checkout afc9e88a4d

 Documentation/gitattributes.txt |  27 ++
 convert.c   | 192 +++-
 t/t0028-encoding.sh |  60 +
 3 files changed, 278 insertions(+), 1 deletion(-)
 create mode 100755 t/t0028-encoding.sh

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 30687de81a..84de2fa49c 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -146,6 +146,33 @@ Unspecified::
 Any other value causes Git to act as if `text` has been left
 unspecified.

+`encoding`
+^^
+
+By default Git assumes UTF-8 encoding for text files.  The `encoding`
+attribute sets the encoding to be used in the working directory.
+If the path is added to the index, then Git encodes the content to
+UTF-8.  On checkout the content is encoded back to the original
+encoding.  Consequently, you can use all built-in Git text processing
+tools (e.g. git diff, line ending conversions, etc.) with your
+non-UTF-8 encoded file.
+
+Please note that re-encoding content can cause errors and requires
+resources. Use the `encoding` attribute only if you cannot store
+a file in UTF-8 encoding and if you want Git to be able to process
+the text.
+
+
+*.txt  text encoding=UTF-16
+
+
+All `iconv` encodings with a stable round-trip conversion to and from
+UTF-8 are supported.  You can see a full list with the following command:
+
+
+iconv --list
+
+
 `eol`
 ^

diff --git a/convert.c b/convert.c
index 20d7ab67bd..ee19c17104 100644
--- a/convert.c
+++ b/convert.c
@@ -7,6 +7,7 @@
 #include "sigchain.h"
 #include "pkt-line.h"
 #include "sub-process.h"
+#include "utf8.h"

 /*
  * convert.c - convert a file when checking it out and checking it in.
@@ -256,6 +257,149 @@ static int will_convert_lf_to_crlf(size_t len, struct 
text_stat *stats,

 }

+#ifdef NO_ICONV
+#ifndef _ICONV_T
+/* The type is just a placeholder and not actually used. */
+typedef void* iconv_t;
+#endif
+#endif
+
+static struct encoding {
+   const char *name;
+   

Re: [PATCH] builtin/tag.c: return appropriate value when --points-at finds an empty list

[George Papanikolaou]

I agree with what you're saying, just I thought this might be ultra-minor for
API-breakage. To me, 0 doesn't necessarily mean "I didn't segfault".
I lot of tools use ret-values to give information back. And that way it's much
easier to just `||` the command to something else instead of `[[ -z ]]` in the
script.

But I see what you're saying...
--
/ΓΠ


On Mon, Dec 11, 2017 at 4:05 PM, Derrick Stolee  wrote:
> On 12/11/2017 8:44 AM, George Papanikolaou wrote:
>>
>> `git tag --points-at` can simply return if the given rev does not have
>> any tags pointing to it. It's not a failure but it shouldn't return
>> with 0 value.
>
>
> I disagree. I think the 0 return means "I completed successfully" and the
> empty output means "I didn't find any tags pointing to this object."
>
> Changing the return value here could break a lot of scripts out in the wild,
> and I consider this to be an "API" compatibility that needs to stay as-is.
>
> What are you using "--points-at" where you need a nonzero exit code instead
> of a different indicator?
>
> Thanks,
> -Stolee
>

[PATCH v5 7/9] sequencer: load commit related config

[Phillip Wood]

From: Phillip Wood 

Load default values for message cleanup, gpg signing of commits and
basic diff configuration in preparation for committing without forking
'git commit'. Note that we interpret commit.cleanup=scissors to mean
COMMIT_MSG_CLEANUP_SPACE to be consistent with 'git commit'.

The sequencer should probably have been calling
git_diff_basic_config() before as it creates a patch when there are
conflicts. The shell version uses 'diff-index' to create the patch so
calling git_diff_basic_config() should match that. Although 'git
commit' calls git_diff_ui_config() I don't think the output of
print_commit_summary() is affected by anything that is loaded by that
as print_commit_summary() always turns on rename detection so would
ignore the value in the user's configuration anyway. The other values
loaded by git_diff_ui_config() are about the formatting of patches so
are not relevant to print_commit_summary().

Signed-off-by: Phillip Wood 
---

Notes:
changes since v4:
 - reworked config handling to call git_diff_basic_config() and store
   defaults in struct replay_opts rather than using global variables.
 - added a warning if there is an invalid value for commit.cleanup.

changes since v3:
 - interpret commit.cleanup=scissors to mean COMMIT_MSG_CLEANUP_SPACE
   to match 'git commit'

changes since v1:
 - renamed git_revert_config() to common_config()
 - prefixed cleanup_mode constants to reflect the changes to patch 2
   in this series

 builtin/rebase--helper.c |  2 +-
 builtin/revert.c |  4 ++--
 sequencer.c  | 45 +
 sequencer.h  |  3 +++
 4 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 
f8519363a393862b6857acab037e74367c7f2134..decb8f7a09e42eb94bed264164985e54e13a32f6
 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -39,7 +39,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
OPT_END()
};
 
-   git_config(git_default_config, NULL);
+   sequencer_init_config();
 
opts.action = REPLAY_INTERACTIVE_REBASE;
opts.allow_ff = 1;
diff --git a/builtin/revert.c b/builtin/revert.c
index 
b9d927eb09c9ed87c84681df1396f4e6d9b13c97..76f0a35b074b858ab4cb3e3894bc7c877401b7e8
 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -208,7 +208,7 @@ int cmd_revert(int argc, const char **argv, const char 
*prefix)
if (isatty(0))
opts.edit = 1;
opts.action = REPLAY_REVERT;
-   git_config(git_default_config, NULL);
+   sequencer_init_config();
res = run_sequencer(argc, argv, );
if (res < 0)
die(_("revert failed"));
@@ -221,7 +221,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char 
*prefix)
int res;
 
opts.action = REPLAY_PICK;
-   git_config(git_default_config, NULL);
+   sequencer_init_config();
res = run_sequencer(argc, argv, );
if (res < 0)
die(_("cherry-pick failed"));
diff --git a/sequencer.c b/sequencer.c
index 
4966dd1b9359aaa82064608c05a7f5b18cea2d7a..3ce1e5b71474f1cd25b232a319fb7b0e13dc6e14
 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -132,6 +132,51 @@ static GIT_PATH_FUNC(rebase_path_strategy, 
"rebase-merge/strategy")
 static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
 static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, 
"rebase-merge/allow_rerere_autoupdate")
 
+static int git_sequencer_config(const char *k, const char *v, void *cb)
+{
+   struct replay_opts *opts = cb;
+   int status;
+
+   if (!strcmp(k, "commit.cleanup")) {
+   const char *s;
+
+   status = git_config_string(, k, v);
+   if (status)
+   return status;
+
+   if (!strcmp(s, "verbatim"))
+   opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_NONE;
+   else if (!strcmp(s, "whitespace"))
+   opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE;
+   else if (!strcmp(s, "strip"))
+   opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_ALL;
+   else if (!strcmp(s, "scissors"))
+   opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE;
+   else
+   warning(_("invalid commit message cleanup mode '%s'"),
+ s);
+
+   return status;
+   }
+
+   if (!strcmp(k, "commit.gpgsign")) {
+   opts->gpg_sign = git_config_bool(k, v) ? "" : NULL;
+   return 0;
+   }
+
+   status = git_gpg_config(k, v, NULL);
+   if (status)
+   return status;
+
+   return git_diff_basic_config(k, v, NULL);
+}
+
+void 

[PATCH v5 8/9] sequencer: try to commit without forking 'git commit'

[Phillip Wood]

From: Phillip Wood 

If the commit message does not need to be edited then create the
commit without forking 'git commit'. Taking the best time of ten runs
with a warm cache this reduces the time taken to cherry-pick 10
commits by 27% (from 282ms to 204ms), and the time taken by 'git
rebase --continue' to pick 10 commits by 45% (from 386ms to 212ms) on
my computer running linux. Some of greater saving for rebase is
because it no longer wastes time creating the commit summary just to
throw it away.

The code to create the commit is based on builtin/commit.c. It is
simplified as it doesn't have to deal with merges and modified so that
it does not die but returns an error to make sure the sequencer exits
cleanly, as it would when forking 'git commit'

Even when not forking 'git commit' the commit message is written to a
file and CHERRY_PICK_HEAD is created unnecessarily. This could be
eliminated in future. I hacked up a version that does not write these
files and just passed an strbuf (with the wrong message for fixup and
squash commands) to do_commit() but I couldn't measure any significant
time difference when running cherry-pick or rebase. I think
eliminating the writes properly for rebase would require a bit of
effort as the code would need to be restructured.

Signed-off-by: Phillip Wood 
---

Notes:
changes since v4:
 - changed cleanup and gpg handling to reflect the changes in the last patch

changes since v3:
 - take account of change print_commit_summary() return type after
   dropping the patch that made it return an error instead of dying.

changes since v2:
 - style fixes

changes since v1:
 - added comments to explain return value of try_to_commit()
 - removed unnecessary NULL tests before calling free()
 - style cleanups
 - corrected commit message
 - prefixed cleanup_mode constants to reflect the changes to patch 2
   in this series

 sequencer.c | 176 +++-
 1 file changed, 174 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 
3ce1e5b71474f1cd25b232a319fb7b0e13dc6e14..74770bd00cc3840573057a1868e0a3acb05a71bb
 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -638,6 +638,18 @@ static int read_env_script(struct argv_array *env)
return 0;
 }
 
+static char *get_author(const char *message)
+{
+   size_t len;
+   const char *a;
+
+   a = find_commit_header(message, "author", );
+   if (a)
+   return xmemdupz(a, len);
+
+   return NULL;
+}
+
 static const char staged_changes_advice[] =
 N_("you have staged changes in your working tree\n"
 "If these changes are meant to be squashed into the previous commit, run:\n"
@@ -996,6 +1008,158 @@ void print_commit_summary(const char *prefix, const 
struct object_id *oid,
strbuf_release();
 }
 
+static int parse_head(struct commit **head)
+{
+   struct commit *current_head;
+   struct object_id oid;
+
+   if (get_oid("HEAD", )) {
+   current_head = NULL;
+   } else {
+   current_head = lookup_commit_reference();
+   if (!current_head)
+   return error(_("could not parse HEAD"));
+   if (oidcmp(, _head->object.oid)) {
+   warning(_("HEAD %s is not a commit!"),
+   oid_to_hex());
+   }
+   if (parse_commit(current_head))
+   return error(_("could not parse HEAD commit"));
+   }
+   *head = current_head;
+
+   return 0;
+}
+
+/*
+ * Try to commit without forking 'git commit'. In some cases we need
+ * to run 'git commit' to display an error message
+ *
+ * Returns:
+ *  -1 - error unable to commit
+ *   0 - success
+ *   1 - run 'git commit'
+ */
+static int try_to_commit(struct strbuf *msg, const char *author,
+struct replay_opts *opts, unsigned int flags,
+struct object_id *oid)
+{
+   struct object_id tree;
+   struct commit *current_head;
+   struct commit_list *parents = NULL;
+   struct commit_extra_header *extra = NULL;
+   struct strbuf err = STRBUF_INIT;
+   struct strbuf amend_msg = STRBUF_INIT;
+   char *amend_author = NULL;
+   enum commit_msg_cleanup_mode cleanup;
+   int res = 0;
+
+   if (parse_head(_head))
+   return -1;
+
+   if (flags & AMEND_MSG) {
+   const char *exclude_gpgsig[] = { "gpgsig", NULL };
+   const char *out_enc = get_commit_output_encoding();
+   const char *message = logmsg_reencode(current_head, NULL,
+ out_enc);
+
+   if (!msg) {
+   const char *orig_message = NULL;
+
+   find_commit_subject(message, _message);
+   msg = _msg;
+  

[PATCH v5 9/9] t3512/t3513: remove KNOWN_FAILURE_CHERRY_PICK_SEES_EMPTY_COMMIT=1

[Phillip Wood]

From: Phillip Wood 

Now that the sequencer creates commits without forking 'git commit' it
does not see an empty commit in these tests which fixes the known
breakage. Note that logic for handling
KNOWN_FAILURE_CHERRY_PICK_SEES_EMPTY_COMMIT=1 is not removed from
lib-submodule-update.sh as it is still used by other tests.

Reviewed-by: Stefan Beller 
Signed-off-by: Phillip Wood 
---

Notes:
Changes since v4
 - Added reviewed-by trailer

 t/t3512-cherry-pick-submodule.sh | 1 -
 t/t3513-revert-submodule.sh  | 1 -
 2 files changed, 2 deletions(-)

diff --git a/t/t3512-cherry-pick-submodule.sh b/t/t3512-cherry-pick-submodule.sh
index 
ce48c4fcca80b183927292cc1e5902cfe286f994..bd78287841ee053fd56a44a268f8077a222cc266
 100755
--- a/t/t3512-cherry-pick-submodule.sh
+++ b/t/t3512-cherry-pick-submodule.sh
@@ -5,7 +5,6 @@ test_description='cherry-pick can handle submodules'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-submodule-update.sh
 
-KNOWN_FAILURE_CHERRY_PICK_SEES_EMPTY_COMMIT=1
 KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
 KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
 test_submodule_switch "git cherry-pick"
diff --git a/t/t3513-revert-submodule.sh b/t/t3513-revert-submodule.sh
index 
db9378142a93338d2988f40e2748bc476490bcd5..5e39fcdb66c0c7c4b112c1bbe941d886db237693
 100755
--- a/t/t3513-revert-submodule.sh
+++ b/t/t3513-revert-submodule.sh
@@ -25,7 +25,6 @@ git_revert () {
git revert HEAD
 }
 
-KNOWN_FAILURE_CHERRY_PICK_SEES_EMPTY_COMMIT=1
 KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
 test_submodule_switch "git_revert"
 
-- 
2.15.1

[PATCH v5 6/9] sequencer: simplify adding Signed-off-by: trailer

[Phillip Wood]

From: Phillip Wood 

Add the Signed-off-by: trailer in one place rather than adding it to
the message when doing a recursive merge and specifying '--signoff'
when running 'git commit'. This means that if there are conflicts when
merging with a strategy other than 'recursive' the Signed-off-by:
trailer will be added if the user commits the resolution themselves
without passing '--signoff' to 'git commit'. It also simplifies the
in-process commit that is about to be added to the sequencer.

Signed-off-by: Phillip Wood 
---
 sequencer.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 
b4ff2a4a973b2733cca7bb65fcb7947cb8d08988..4966dd1b9359aaa82064608c05a7f5b18cea2d7a
 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -478,9 +478,6 @@ static int do_recursive_merge(struct commit *base, struct 
commit *next,
_(action_name(opts)));
rollback_lock_file(_lock);
 
-   if (opts->signoff)
-   append_signoff(msgbuf, 0, 0);
-
if (!clean)
append_conflicts_hint(msgbuf);
 
@@ -658,8 +655,6 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
argv_array_push(, "--amend");
if (opts->gpg_sign)
argv_array_pushf(, "-S%s", opts->gpg_sign);
-   if (opts->signoff)
-   argv_array_push(, "-s");
if (defmsg)
argv_array_pushl(, "-F", defmsg, NULL);
if ((flags & CLEANUP_MSG))
@@ -1348,6 +1343,9 @@ static int do_pick_commit(enum todo_command command, 
struct commit *commit,
}
}
 
+   if (opts->signoff)
+   append_signoff(, 0, 0);
+
if (is_rebase_i(opts) && write_author_script(msg.message) < 0)
res = -1;
else if (!opts->strategy || !strcmp(opts->strategy, "recursive") || 
command == TODO_REVERT) {
-- 
2.15.1

[PATCH v5 5/9] commit: move print_commit_summary() to libgit

[Phillip Wood]

From: Phillip Wood 

Move print_commit_summary() from builtin/commit.c to sequencer.c so it
can be shared with other commands. The function is modified by
changing the last argument to a flag so callers can specify whether
they want to show the author date in addition to specifying if this is
an initial commit.

If the sequencer dies in print_commit_summary() (which can only happen
when cherry-picking or reverting) then neither the todo list nor the
abort safety file are updated to reflect the commit that was just
made. print_commit_summary() can die if:

 - The commit that was just created cannot be found or parsed.

 - HEAD cannot be resolved either because some other process is
   updating it (which is bad news in the middle of a cherry-pick) or
   because it is corrupt.

 - log_tree_commit() cannot read some objects.

In all those cases dying will leave the sequencer in a sane state for
aborting; 'git cherry-pick --abort' will rewind HEAD to the last
successful commit before there was a problem with HEAD or the object
database. If the user somehow fixes the problem and runs 'git
cherry-pick --continue' then the sequencer will try and pick the same
commit again which may or may not be what the user wants depending on
what caused print_commit_summary() to die. If print_commit_summary()
returned an error instead then update_abort_safety_file() would try to
resolve HEAD which may or may not be successful. If it is successful
then running 'git rebase --abort' would not rewind HEAD to the last
successful commit which is not what we want.

Signed-off-by: Phillip Wood 
---

Notes:
changes since v2:
 - expanded commit message to explain why it is ok to die in
   print_commit_summary() and dropped the next patch which made it
   return an error instead.
 - style fixes.

changes since v1:
 - convert flags passed to print_commit_summary() to unsigned int

 builtin/commit.c | 128 ---
 sequencer.c  | 119 +++
 sequencer.h  |   5 +++
 3 files changed, 133 insertions(+), 119 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 
d376d282cda95d491f2fcbee5063709d02aed77a..0f78958c89cdd91c54f0154102b08e1a8121cce0
 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -43,31 +43,6 @@ static const char * const builtin_status_usage[] = {
NULL
 };
 
-static const char implicit_ident_advice_noconfig[] =
-N_("Your name and email address were configured automatically based\n"
-"on your username and hostname. Please check that they are accurate.\n"
-"You can suppress this message by setting them explicitly. Run the\n"
-"following command and follow the instructions in your editor to edit\n"
-"your configuration file:\n"
-"\n"
-"git config --global --edit\n"
-"\n"
-"After doing this, you may fix the identity used for this commit with:\n"
-"\n"
-"git commit --amend --reset-author\n");
-
-static const char implicit_ident_advice_config[] =
-N_("Your name and email address were configured automatically based\n"
-"on your username and hostname. Please check that they are accurate.\n"
-"You can suppress this message by setting them explicitly:\n"
-"\n"
-"git config --global user.name \"Your Name\"\n"
-"git config --global user.email y...@example.com\n"
-"\n"
-"After doing this, you may fix the identity used for this commit with:\n"
-"\n"
-"git commit --amend --reset-author\n");
-
 static const char empty_amend_advice[] =
 N_("You asked to amend the most recent commit, but doing so would make\n"
 "it empty. You can repeat your command with --allow-empty, or you can\n"
@@ -1374,98 +1349,6 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
return 0;
 }
 
-static const char *implicit_ident_advice(void)
-{
-   char *user_config = expand_user_path("~/.gitconfig", 0);
-   char *xdg_config = xdg_config_home("config");
-   int config_exists = file_exists(user_config) || file_exists(xdg_config);
-
-   free(user_config);
-   free(xdg_config);
-
-   if (config_exists)
-   return _(implicit_ident_advice_config);
-   else
-   return _(implicit_ident_advice_noconfig);
-
-}
-
-static void print_summary(const char *prefix, const struct object_id *oid,
- int initial_commit)
-{
-   struct rev_info rev;
-   struct commit *commit;
-   struct strbuf format = STRBUF_INIT;
-   const char *head;
-   struct pretty_print_context pctx = {0};
-   struct strbuf author_ident = STRBUF_INIT;
-   struct strbuf committer_ident = STRBUF_INIT;
-
-   commit = lookup_commit(oid);
-   if (!commit)
-   die(_("couldn't look up newly created commit"));
-   if (parse_commit(commit))
-   die(_("could not parse newly created commit"));
-
-   strbuf_addstr(, "format:%h] %s");
-
- 

[PATCH v5 4/9] commit: move post-rewrite code to libgit

[Phillip Wood]

From: Phillip Wood 

Move run_rewrite_hook() from bulitin/commit.c to sequencer.c so it can
be shared with other commands and add a new function
commit_post_rewrite() based on the code in builtin/commit.c that
encapsulates rewriting notes and running the post-rewrite hook. Once
the sequencer learns how to create commits without forking 'git
commit' these functions will be used when squashing commits.

Signed-off-by: Phillip Wood 
---

Notes:
changes since v1:
 - reword commit message to explain why the code is being moved

 builtin/commit.c | 42 +-
 sequencer.c  | 47 +++
 sequencer.h  |  2 ++
 3 files changed, 50 insertions(+), 41 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 
340cc2988ebdb92ef222b677ee12c94fa53aa1ff..d376d282cda95d491f2fcbee5063709d02aed77a
 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -31,9 +31,7 @@
 #include "gpg-interface.h"
 #include "column.h"
 #include "sequencer.h"
-#include "notes-utils.h"
 #include "mailmap.h"
-#include "sigchain.h"
 
 static const char * const builtin_commit_usage[] = {
N_("git commit [] [--] ..."),
@@ -1497,37 +1495,6 @@ static int git_commit_config(const char *k, const char 
*v, void *cb)
return git_status_config(k, v, s);
 }
 
-static int run_rewrite_hook(const struct object_id *oldoid,
-   const struct object_id *newoid)
-{
-   struct child_process proc = CHILD_PROCESS_INIT;
-   const char *argv[3];
-   int code;
-   struct strbuf sb = STRBUF_INIT;
-
-   argv[0] = find_hook("post-rewrite");
-   if (!argv[0])
-   return 0;
-
-   argv[1] = "amend";
-   argv[2] = NULL;
-
-   proc.argv = argv;
-   proc.in = -1;
-   proc.stdout_to_stderr = 1;
-
-   code = start_command();
-   if (code)
-   return code;
-   strbuf_addf(, "%s %s\n", oid_to_hex(oldoid), oid_to_hex(newoid));
-   sigchain_push(SIGPIPE, SIG_IGN);
-   write_in_full(proc.in, sb.buf, sb.len);
-   close(proc.in);
-   strbuf_release();
-   sigchain_pop(SIGPIPE);
-   return finish_command();
-}
-
 int run_commit_hook(int editor_is_used, const char *index_file, const char 
*name, ...)
 {
struct argv_array hook_env = ARGV_ARRAY_INIT;
@@ -1758,14 +1725,7 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
rerere(0);
run_commit_hook(use_editor, get_index_file(), "post-commit", NULL);
if (amend && !no_post_rewrite) {
-   struct notes_rewrite_cfg *cfg;
-   cfg = init_copy_notes_for_rewrite("amend");
-   if (cfg) {
-   /* we are amending, so current_head is not NULL */
-   copy_note_for_rewrite(cfg, _head->object.oid, 
);
-   finish_copy_notes_for_rewrite(cfg, "Notes added by 'git 
commit --amend'");
-   }
-   run_rewrite_hook(_head->object.oid, );
+   commit_post_rewrite(current_head, );
}
if (!quiet)
print_summary(prefix, , !current_head);
diff --git a/sequencer.c b/sequencer.c
index 
5fe6ef3512566622f0423a09cbffe1adf1e65957..132319be0dbc15ed34f87af769235a4c6f3c6159
 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -21,6 +21,8 @@
 #include "log-tree.h"
 #include "wt-status.h"
 #include "hashmap.h"
+#include "notes-utils.h"
+#include "sigchain.h"
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
@@ -790,6 +792,51 @@ int update_head_with_reflog(const struct commit *old_head,
return ret;
 }
 
+static int run_rewrite_hook(const struct object_id *oldoid,
+   const struct object_id *newoid)
+{
+   struct child_process proc = CHILD_PROCESS_INIT;
+   const char *argv[3];
+   int code;
+   struct strbuf sb = STRBUF_INIT;
+
+   argv[0] = find_hook("post-rewrite");
+   if (!argv[0])
+   return 0;
+
+   argv[1] = "amend";
+   argv[2] = NULL;
+
+   proc.argv = argv;
+   proc.in = -1;
+   proc.stdout_to_stderr = 1;
+
+   code = start_command();
+   if (code)
+   return code;
+   strbuf_addf(, "%s %s\n", oid_to_hex(oldoid), oid_to_hex(newoid));
+   sigchain_push(SIGPIPE, SIG_IGN);
+   write_in_full(proc.in, sb.buf, sb.len);
+   close(proc.in);
+   strbuf_release();
+   sigchain_pop(SIGPIPE);
+   return finish_command();
+}
+
+void commit_post_rewrite(const struct commit *old_head,
+const struct object_id *new_head)
+{
+   struct notes_rewrite_cfg *cfg;
+
+   cfg = init_copy_notes_for_rewrite("amend");
+   if (cfg) {
+   /* we are amending, so old_head is not NULL */
+   copy_note_for_rewrite(cfg, _head->object.oid, new_head);
+   finish_copy_notes_for_rewrite(cfg, "Notes added by 'git 

[PATCH v5 2/9] commit: move empty message checks to libgit

[Phillip Wood]

From: Phillip Wood 

Move the functions that check for empty messages from bulitin/commit.c
to sequencer.c so they can be shared with other commands. The
functions are refactored to take an explicit cleanup mode and template
filename passed by the caller.

Signed-off-by: Phillip Wood 
---

Notes:
changes since v4:
 - move the definition of cleanup mode enum so it can be referenced in
   struct replay_opts by a later commit

changes since v1:
 - prefix cleanup_mode enum and constants with commit_msg_

 builtin/commit.c | 99 +++-
 sequencer.c  | 61 ++
 sequencer.h  | 12 ++-
 3 files changed, 91 insertions(+), 81 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 
2de75882b847be1a100931f6218e7e14dc01c4bd..2175dac8036c465a73c4c782f061e85ae6d1a629
 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -128,12 +128,7 @@ static char *sign_commit;
  * if editor is used, and only the whitespaces if the message
  * is specified explicitly.
  */
-static enum {
-   CLEANUP_SPACE,
-   CLEANUP_NONE,
-   CLEANUP_SCISSORS,
-   CLEANUP_ALL
-} cleanup_mode;
+static enum commit_msg_cleanup_mode cleanup_mode;
 static const char *cleanup_arg;
 
 static enum commit_whence whence;
@@ -673,7 +668,7 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
struct strbuf sb = STRBUF_INIT;
const char *hook_arg1 = NULL;
const char *hook_arg2 = NULL;
-   int clean_message_contents = (cleanup_mode != CLEANUP_NONE);
+   int clean_message_contents = (cleanup_mode != COMMIT_MSG_CLEANUP_NONE);
int old_display_comment_prefix;
 
/* This checks and barfs if author is badly specified */
@@ -812,7 +807,7 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
struct ident_split ci, ai;
 
if (whence != FROM_COMMIT) {
-   if (cleanup_mode == CLEANUP_SCISSORS)
+   if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)
wt_status_add_cut_line(s->fp);
status_printf_ln(s, GIT_COLOR_NORMAL,
whence == FROM_MERGE
@@ -832,14 +827,15 @@ static int prepare_to_commit(const char *index_file, 
const char *prefix,
}
 
fprintf(s->fp, "\n");
-   if (cleanup_mode == CLEANUP_ALL)
+   if (cleanup_mode == COMMIT_MSG_CLEANUP_ALL)
status_printf(s, GIT_COLOR_NORMAL,
_("Please enter the commit message for your 
changes."
  " Lines starting\nwith '%c' will be ignored, 
and an empty"
  " message aborts the commit.\n"), 
comment_line_char);
-   else if (cleanup_mode == CLEANUP_SCISSORS && whence == 
FROM_COMMIT)
+   else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
+whence == FROM_COMMIT)
wt_status_add_cut_line(s->fp);
-   else /* CLEANUP_SPACE, that is. */
+   else /* COMMIT_MSG_CLEANUP_SPACE, that is. */
status_printf(s, GIT_COLOR_NORMAL,
_("Please enter the commit message for your 
changes."
  " Lines starting\n"
@@ -984,65 +980,6 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
return 1;
 }
 
-static int rest_is_empty(struct strbuf *sb, int start)
-{
-   int i, eol;
-   const char *nl;
-
-   /* Check if the rest is just whitespace and Signed-off-by's. */
-   for (i = start; i < sb->len; i++) {
-   nl = memchr(sb->buf + i, '\n', sb->len - i);
-   if (nl)
-   eol = nl - sb->buf;
-   else
-   eol = sb->len;
-
-   if (strlen(sign_off_header) <= eol - i &&
-   starts_with(sb->buf + i, sign_off_header)) {
-   i = eol;
-   continue;
-   }
-   while (i < eol)
-   if (!isspace(sb->buf[i++]))
-   return 0;
-   }
-
-   return 1;
-}
-
-/*
- * Find out if the message in the strbuf contains only whitespace and
- * Signed-off-by lines.
- */
-static int message_is_empty(struct strbuf *sb)
-{
-   if (cleanup_mode == CLEANUP_NONE && sb->len)
-   return 0;
-   return rest_is_empty(sb, 0);
-}
-
-/*
- * See if the user edited the message in the editor or left what
- * was in the template intact
- */
-static int template_untouched(struct strbuf *sb)
-{
-   struct strbuf tmpl = STRBUF_INIT;
-   const char *start;
-
-   if (cleanup_mode == CLEANUP_NONE && sb->len)
-   return 0;

[PATCH v5 3/9] Add a function to update HEAD after creating a commit

[Phillip Wood]

From: Phillip Wood 

Add update_head_with_reflog() based on the code that updates HEAD
after committing in builtin/commit.c that can be called by 'git
commit' and other commands.

Signed-off-by: Phillip Wood 
---

Notes:
changes since v2:
 - updated commit message to reflect the change in function name
 - style fixes

changes since v1:
 - rename update_head() to update_head_with_reflog()

 builtin/commit.c | 20 ++--
 sequencer.c  | 39 ++-
 sequencer.h  |  4 
 3 files changed, 44 insertions(+), 19 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 
2175dac8036c465a73c4c782f061e85ae6d1a629..340cc2988ebdb92ef222b677ee12c94fa53aa1ff
 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1610,13 +1610,11 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
struct strbuf sb = STRBUF_INIT;
struct strbuf author_ident = STRBUF_INIT;
const char *index_file, *reflog_msg;
-   char *nl;
struct object_id oid;
struct commit_list *parents = NULL;
struct stat statbuf;
struct commit *current_head = NULL;
struct commit_extra_header *extra = NULL;
-   struct ref_transaction *transaction;
struct strbuf err = STRBUF_INIT;
 
if (argc == 2 && !strcmp(argv[1], "-h"))
@@ -1739,25 +1737,11 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
strbuf_release(_ident);
free_commit_extra_headers(extra);
 
-   nl = strchr(sb.buf, '\n');
-   if (nl)
-   strbuf_setlen(, nl + 1 - sb.buf);
-   else
-   strbuf_addch(, '\n');
-   strbuf_insert(, 0, reflog_msg, strlen(reflog_msg));
-   strbuf_insert(, strlen(reflog_msg), ": ", 2);
-
-   transaction = ref_transaction_begin();
-   if (!transaction ||
-   ref_transaction_update(transaction, "HEAD", ,
-  current_head
-  ? _head->object.oid : _oid,
-  0, sb.buf, ) ||
-   ref_transaction_commit(transaction, )) {
+   if (update_head_with_reflog(current_head, , reflog_msg, ,
+   )) {
rollback_index_files();
die("%s", err.buf);
}
-   ref_transaction_free(transaction);
 
unlink(git_path_cherry_pick_head());
unlink(git_path_revert_head());
diff --git a/sequencer.c b/sequencer.c
index 
168da5093e71f50a4d70af7288cf761110e69e87..5fe6ef3512566622f0423a09cbffe1adf1e65957
 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1,10 +1,10 @@
 #include "cache.h"
 #include "config.h"
 #include "lockfile.h"
-#include "sequencer.h"
 #include "dir.h"
 #include "object.h"
 #include "commit.h"
+#include "sequencer.h"
 #include "tag.h"
 #include "run-command.h"
 #include "exec_cmd.h"
@@ -753,6 +753,43 @@ int template_untouched(const struct strbuf *sb, const char 
*template_file,
return rest_is_empty(sb, start - sb->buf);
 }
 
+int update_head_with_reflog(const struct commit *old_head,
+   const struct object_id *new_head,
+   const char *action, const struct strbuf *msg,
+   struct strbuf *err)
+{
+   struct ref_transaction *transaction;
+   struct strbuf sb = STRBUF_INIT;
+   const char *nl;
+   int ret = 0;
+
+   if (action) {
+   strbuf_addstr(, action);
+   strbuf_addstr(, ": ");
+   }
+
+   nl = strchr(msg->buf, '\n');
+   if (nl) {
+   strbuf_add(, msg->buf, nl + 1 - msg->buf);
+   } else {
+   strbuf_addbuf(, msg);
+   strbuf_addch(, '\n');
+   }
+
+   transaction = ref_transaction_begin(err);
+   if (!transaction ||
+   ref_transaction_update(transaction, "HEAD", new_head,
+  old_head ? _head->object.oid : _oid,
+  0, sb.buf, err) ||
+   ref_transaction_commit(transaction, err)) {
+   ret = -1;
+   }
+   ref_transaction_free(transaction);
+   strbuf_release();
+
+   return ret;
+}
+
 static int is_original_commit_empty(struct commit *commit)
 {
const struct object_id *ptree_oid;
diff --git a/sequencer.h b/sequencer.h
index 
2040773c7b6fd3966d5a1a14f410ea8ff53843c8..3757a7aecb5a7795d7b9b45964f3328ee852e14b
 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -68,4 +68,8 @@ int message_is_empty(const struct strbuf *sb,
 enum commit_msg_cleanup_mode cleanup_mode);
 int template_untouched(const struct strbuf *sb, const char *template_file,
   enum commit_msg_cleanup_mode cleanup_mode);
+int update_head_with_reflog(const struct commit *old_head,
+   const struct object_id *new_head,
+   const char 

[PATCH v5 1/9] t3404: check intermediate squash messages

[Phillip Wood]

From: Phillip Wood 

When there is more than one squash/fixup command in a row check the
intermediate messages are correct.

Signed-off-by: Phillip Wood 
---
 t/t3404-rebase-interactive.sh | 4 
 1 file changed, 4 insertions(+)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 
6a82d1ed876dd5d1073dc63be8ba5720adbf12e3..9ed0a244e6cdf34c7caca8232f0c0a8cf4864c42
 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -453,6 +453,10 @@ test_expect_success C_LOCALE_OUTPUT 'squash and fixup 
generate correct log messa
git rebase -i $base &&
git cat-file commit HEAD | sed -e 1,/^\$/d > actual-squash-fixup &&
test_cmp expect-squash-fixup actual-squash-fixup &&
+   git cat-file commit HEAD@{2} |
+   grep "^# This is a combination of 3 commits\."  &&
+   git cat-file commit HEAD@{3} |
+   grep "^# This is a combination of 2 commits\."  &&
git checkout to-be-rebased &&
git branch -D squash-fixup
 '
-- 
2.15.1

[PATCH v5 0/9] sequencer: don't fork git commit

[Phillip Wood]

From: Phillip Wood 

I've reworked the config handling since v4. It now stores the default
values in struct replay_opt rather than using global variables and
calls git_diff_basic_config(). Unfortunately I've not had time to
modify git_gpg_config() to indicate if it successfully handled the key
so git_diff_basic_config() is called unnecessarily in that case. Within
git_diff_basic_config() userdiff_config() also suffers from the same
problem of not indicating if it has handled the key.

Here's the original summary:
These patches teach the sequencer to create commits without forking
git commit when the commit message does not need to be edited. This
speeds up cherry picking 10 commits by 26% and picking 10 commits with
rebase --continue by 44%. The first few patches move bits of
builtin/commit.c to sequencer.c. The last two patches actually
implement creating commits in sequencer.c.

Phillip Wood (9):
  t3404: check intermediate squash messages
  commit: move empty message checks to libgit
  Add a function to update HEAD after creating a commit
  commit: move post-rewrite code to libgit
  commit: move print_commit_summary() to libgit
  sequencer: simplify adding Signed-off-by: trailer
  sequencer: load commit related config
  sequencer: try to commit without forking 'git commit'
  t3512/t3513: remove KNOWN_FAILURE_CHERRY_PICK_SEES_EMPTY_COMMIT=1

 builtin/commit.c | 289 +++
 builtin/rebase--helper.c |   2 +-
 builtin/revert.c |   4 +-
 sequencer.c  | 495 ++-
 sequencer.h  |  24 ++
 t/t3404-rebase-interactive.sh|   4 +
 t/t3512-cherry-pick-submodule.sh |   1 -
 t/t3513-revert-submodule.sh  |   1 -
 8 files changed, 549 insertions(+), 271 deletions(-)

-- 
2.15.1

Re: [PATCH] builtin/tag.c: return appropriate value when --points-at finds an empty list

[Derrick Stolee]

On 12/11/2017 8:44 AM, George Papanikolaou wrote:

`git tag --points-at` can simply return if the given rev does not have
any tags pointing to it. It's not a failure but it shouldn't return
with 0 value.


I disagree. I think the 0 return means "I completed successfully" and 
the empty output means "I didn't find any tags pointing to this object."


Changing the return value here could break a lot of scripts out in the 
wild, and I consider this to be an "API" compatibility that needs to 
stay as-is.


What are you using "--points-at" where you need a nonzero exit code 
instead of a different indicator?


Thanks,
-Stolee

[PATCH] builtin/tag.c: return appropriate value when --points-at finds an empty list

[George Papanikolaou]

`git tag --points-at` can simply return if the given rev does not have
any tags pointing to it. It's not a failure but it shouldn't return
with 0 value.
---
 builtin/tag.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/builtin/tag.c b/builtin/tag.c
index b38329b59..68b84db2a 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -58,6 +58,10 @@ static int list_tags(struct ref_filter *filter, struct 
ref_sorting *sorting,
die(_("unable to parse format string"));
filter->with_commit_tag_algo = 1;
filter_refs(, filter, FILTER_REFS_TAGS);
+
+   if (array.nr == 0)
+   return -1;
+
ref_array_sort(sorting, );
 
for (i = 0; i < array.nr; i++)
-- 
2.11.0

Re: How to begin an error/die string? Upper- or lower-case letter?

[Thomas Gummerer]

On Mon, Dec 11, 2017 at 10:23 AM, Lars Schneider
 wrote:
> Hi,
>
> error() and die() messages seems to begin with upper-case and
> lower-case letters in the Git code base:
>
>   git grep 'error(_' | perl -nE 'say /.*error\(_\("(.).*/' | sort | uniq -c
>   git grep 'die(_' | perl -nE 'say /.*die\(_\("(.).*/' | sort | uniq -c
>
> Do we prefer one way over the other?

The coding guidelines mandate not capitalizing error messages:

$ git grep -A6 "Error Messages" Documentation/CodingGuidelines
Documentation/CodingGuidelines:Error Messages
Documentation/CodingGuidelines-
Documentation/CodingGuidelines- - Do not end error messages with a
full stop.
Documentation/CodingGuidelines-
Documentation/CodingGuidelines- - Do not capitalize ("unable to
open %s", not "Unable to open %s")
Documentation/CodingGuidelines-
Documentation/CodingGuidelines- - Say what the error is first
("cannot open %s", not "%s: cannot open")

I guess those that are capitalized are just leftovers from before
we had that guideline in place, or slipped through review.

> Thanks,
> Lars
>

How to begin an error/die string? Upper- or lower-case letter?

[Lars Schneider]

Hi,

error() and die() messages seems to begin with upper-case and 
lower-case letters in the Git code base:

  git grep 'error(_' | perl -nE 'say /.*error\(_\("(.).*/' | sort | uniq -c
  git grep 'die(_' | perl -nE 'say /.*die\(_\("(.).*/' | sort | uniq -c

Do we prefer one way over the other?

Thanks,
Lars

Re: [PATCH Outreachy 1/2] format: create pretty.h file

[Оля Тележная]

Is it true that I need to fix only one commit message? (a typo
s/futher/further/)

Do you have any other advises what do I need to change?

Thanks!

Antw: Re: Re: bug deleting "unmerged" branch (2.12.3)

[Ulrich Windl]

Hi!

Sorry for the late response:
On a somewhat not-up-to date manual:

   -d, --delete
   Delete a branch. The branch must be fully merged in its upstream
   branch, or in HEAD if no upstream was set with --track or
   --set-upstream.


Maybe the topic of multiple branches pointing to the same commit could be 
mentioned (regarding the status of each such branch being considered to be 
merged or not). Also "fully merged" could be made a bit more precise, maybe.

Maybe gitglossary could have definitions for "merged" and "fully merged" with 
manual pages referring to it.

Regards,
Ulrich


>>> "Philip Oakley"  schrieb am 08.12.2017 um 21:26 in
Nachricht <582105F8768F4DA6AF4EC82888F0BFBE@PhilipOakley>:
> From: "Ulrich Windl" 
>> Hi Philip!
>>
>> I'm unsure what you are asking for...
>>
>> Ulrich
> 
> Hi Ulrich,
> 
> I was doing a retrospective follow up (of the second kind [1]).
> 
> In your initial email
> https://public-inbox.org/git/5a1d70fd02a100029...@gwsmtp1.uni-regensburg.d
>  
> e/
> you said
> 
> "I wanted to delete the temporary branch (which is of no use now), I got a
> message that the branch is unmerged.
> I think if more than one branches are pointing to the same commit, one
> should be allowed to delete all but the last one without warning."
> 
> My retrospectives question was to find what what part of the documentation
> could be improved to assist fellow coders and Git users in gaining a better
> understanding here. I think it's an easy mistake [2] to make and that we
> should try to make the man pages more assistive.
> 
> I suspect that the description for the `git branch -d` needs a few more
> words to clarify the 'merged/unmerged' issue for those who recieve the
> warning message. Or maybe the git-glossary, etc. I tend to believe that most
> users will read some of the man pages, and would continue to do so if they
> are useful.
> 
> I'd welcome any feedback or suggestions you could provide.
> --
> Philip
> 
>> >>> "Philip Oakley"  04.12.17 0.30 Uhr >>>
>> From: "Junio C Hamano" 
>> > "Philip Oakley"  writes:
>> >
>> >> I think it was that currently you are on M, and neither A nor B are
>> >> ancestors (i.e. merged) of M.
>> >>
>> >> As Junio said:- "branch -d" protects branches that are yet to be
>> >> merged to the **current branch**.
>> >
>> > Actually, I think people loosened this over time and removal of
>> > branch X is not rejected even if the range HEAD..X is not empty, as
>> > long as X is marked to integrate with/build on something else with
>> > branch.X.{remote,merge} and the range X@{upstream}..X is empty.
>> >
>> > So the stress of "current branch" above you added is a bit of a
>> > white lie.
>>
>> Ah, thanks. [I haven't had chance to check the code]
>>
>> The man page does say:
>> .-d
>> .Delete a branch. The branch must be fully merged in its upstream
>> .branch, or in HEAD if no upstream was set with --track
>> .or --set-upstream.
>>
>> It's whether or not Ulrich had joined the two aspects together, and if the
>> doc was sufficient to help recognise the 'unmerged' issue. Ulrich?
>> --
>> Philip
>>
>>
> 
> [1] Retrospective Second Directive, section 3.4.2 of (15th Ed) Agile
> Processes in software engineering and extreme programming. ISBN 1628251042
> (for the perspective of the retrospective..)
> [2] 'mistake' colloquial part of the error categories of slips lapses and
> mistakes : Human Error, by Reason (James, prof) ISBN 0521314194 (worthwhile)