Re: Is it not bug git stash -- does not work at non-root directory?

[Junio C Hamano]

小川恭史  writes:

>> Please make it a habit (not limited to when interacting with
>> _this_ project) to state a bit more than "does not work";
>> instead, say "it is expected to do X, but instead it does Y, and
>> the difference between X and Y I perceive is Z".
>
> Thanks. I'll rewrite the issue.
>
> Assuming that we have sub/something and something is not included anywhere 
> else,
>
> cd sub && git stash -- something
>
>  is expected to make a stash for sub/something but instead returns error like
>
> error: pathspec 'something' did not match any file(s) known to git.
> Did you forget to 'git add'?
>
> .
>
> I don't know what I should write about 'the difference between X and Y is Z'.

If the difference between X and Y is obvious there is no need.  

I just tried it and I do not see the command is broken in the way
you describe.

Trial #1 -- the command fully spelled out.

$ git.git/master: cd Documentation
$ Documentation/master: echo >>Makefile
$ Documentation/master: git stash push -m "doc-make" -- Makefile
Saved working directory and index state On master: doc-make
$ Documentation/master: git stash show --stat 
 Documentation/Makefile | 1 +
 1 file changed, 1 insertion(+:

Trial #2 -- lazily issue the command without subcommand.

$ git.git/master: cd Documentation
$ Documentation/master: echo >>Makefile
$ Documentation/master: git stash -- Makefile
Saved working directory and index state WIP on master: 89ea799ffc Sync with 
maint
$ Documentation/master: git stash show --stat 
 Documentation/Makefile | 1 +
 1 file changed, 1 insertion(+:

Trial #3 -- make sure having files with the same name is not hiding any bug.

$ git.git/master: cd Documentation
$ Documentation/master: echo >>CodingGuidelines
$ Documentation/master: git stash -- CodingGuidelines
Saved working directory and index state WIP on master: 89ea799ffc
$ Documentation/master: git stash show --stat
 Documentation/CodingGuidelines | 1 +
  1 file changed, 1 insertion(+)

Trial #4 -- simulate a PEBKAC

$ git.git/master: cd Documentation
$ Documentation/master: echo >>no-such-file
$ Documentation/master: git stash -- no-such-file
error: pathspec 'Documentation/no-such-file' did not match any file(s) 
known to git.
Did you forget to 'git add'?

The last one is an expected result---the pathspec given to the
command does not match anything tracked, so without first adding the
file, there is nothing for the command to do.

Re: Is it not bug git stash -- does not work at non-root directory?

[小川恭史]

> Please make it a habit (not limited to when interacting with _this_
project) to state a bit more than "does not work"; instead, say "it
is expected to do X, but instead it does Y, and the difference
between X and Y I perceive is Z".

Thanks. I'll rewrite the issue.

Assuming that we have sub/something and something is not included anywhere else,

cd sub && git stash -- something

 is expected to make a stash for sub/something but instead returns error like

error: pathspec 'something' did not match any file(s) known to git.
Did you forget to 'git add'?

.

I don't know what I should write about 'the difference between X and Y is Z'.

2017-11-18 12:53 GMT+09:00 Junio C Hamano :
> 小川恭史  writes:
>
>> Is it not bug git stash --  does not work at non-root directory?
>
> Please make it a habit (not limited to when interacting with _this_
> project) to state a bit more than "does not work"; instead, say "it
> is expected to do X, but instead it does Y, and the difference
> between X and Y I perceive is Z".
>
> If you mean
>
> cd sub && git stash -- Makefile
>
> does not make a stash for only sub/Makefile and instead makes (or
> attempts to make) a stash for only Makefile at the top-level, then
> I think it is a bug, whose likely cause is that the implementation
> forgets to prepend the $prefix to the pathspec it got from the
> command line.  But I am writing this without looking at the
> implementation and with your unclear description of the issue, so
> I may be completely off the mark ;-)
>
> Thanks.

Re: [PATCH v3 0/8] sequencer: don't fork git commit

[Junio C Hamano]

Junio C Hamano  writes:

> Phillip Wood  writes:
>
>> From: Phillip Wood 
>>
>> I've updated these based on the feedback for v2. I've dropped the
>> patch that stopped print_commit_summary() from dying as I think it is
>> better to die than return an error (see the commit message of the
>> patch that adds print_commit_summary() for the reasoning). Apart from
>> that they're minor changes - style fixes and a reworded a commit message.
>
> Thanks for further polishing this topic; I found nothing in the
> update that was questionable.  Will replace.
>
> With this, perhaps it is ready for 'next'?

Not really.  I needed at least this to get it even compile, which
hints that I do not yet know what _else_ I missed by skimming this
round of the series.

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

diff --git a/sequencer.c b/sequencer.c
index 37460db6b1..63cfb6ddd9 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1139,8 +1139,8 @@ static int do_commit(const char *msg_file, const char 
*author,
unlink(git_path_cherry_pick_head());
unlink(git_path_merge_msg());
if (!is_rebase_i(opts))
-   res = print_commit_summary(NULL, ,
-   SUMMARY_SHOW_AUTHOR_DATE);
+   print_commit_summary(NULL, ,
+SUMMARY_SHOW_AUTHOR_DATE);
return res;
}
}
-- 
2.15.0-372-g9a6f8facfd

Re: Is it not bug git stash -- does not work at non-root directory?

[Junio C Hamano]

小川恭史  writes:

> Is it not bug git stash --  does not work at non-root directory?

Please make it a habit (not limited to when interacting with _this_
project) to state a bit more than "does not work"; instead, say "it
is expected to do X, but instead it does Y, and the difference
between X and Y I perceive is Z".

If you mean

cd sub && git stash -- Makefile

does not make a stash for only sub/Makefile and instead makes (or
attempts to make) a stash for only Makefile at the top-level, then
I think it is a bug, whose likely cause is that the implementation
forgets to prepend the $prefix to the pathspec it got from the
command line.  But I am writing this without looking at the
implementation and with your unclear description of the issue, so
I may be completely off the mark ;-)

Thanks.

Re: [PATCH v3 0/8] sequencer: don't fork git commit

[Junio C Hamano]

Phillip Wood  writes:

> From: Phillip Wood 
>
> I've updated these based on the feedback for v2. I've dropped the
> patch that stopped print_commit_summary() from dying as I think it is
> better to die than return an error (see the commit message of the
> patch that adds print_commit_summary() for the reasoning). Apart from
> that they're minor changes - style fixes and a reworded a commit message.

Thanks for further polishing this topic; I found nothing in the
update that was questionable.  Will replace.

With this, perhaps it is ready for 'next'?

Is it not bug git stash -- does not work at non-root directory?

[小川恭史]

Is it not bug git stash --  does not work at non-root directory?

Re: [PATCH V4] config: add --expiry-date

[Junio C Hamano]

h...@unimetic.com writes:

> diff --git a/config.c b/config.c
> index 903abf953..64f8aa42b 100644
> --- a/config.c
> +++ b/config.c
> @@ -990,6 +990,16 @@ int git_config_pathname(const char **dest, const char 
> *var, const char *value)
>   return 0;
>  }
>  
> +int git_config_expiry_date(timestamp_t *timestamp, const char *var, const 
> char *value)
> +{
> + if (!value)
> + return config_error_nonbool(var);
> + if (parse_expiry_date(value, timestamp))
> + return error(_("'%s' for '%s' is not a valid timestamp"),
> +  value, var);
> + return 0;
> +}
> +

I think this is more correct even within the context of this
function than dying, which suggests the need for a slightly related
(which is not within the scope of this change) clean-up within this
file as a #leftoverbits task.  I think dying in these value parsers
goes against the point of having die_on_error bit in the
config-source structure; Heiko and Peff CC'ed for b2dc0945 ("do not
die when error in config parsing of buf occurs", 2013-07-12).

Thanks; will queue.

[PATCH V4] config: add --expiry-date

[hsed]

From: Haaris Mehmood 

Add --expiry-date as a data-type for config files when
'git config --get' is used. This will return any relative
or fixed dates from config files as timestamps.

This is useful for scripts (e.g. gc.reflogexpire) that work
with timestamps so that '2.weeks' can be converted to a format
acceptable by those scripts/functions.

Following the convention of git_config_pathname(), move
the helper function required for this feature from
builtin/reflog.c to builtin/config.c where other similar
functions exist (e.g. for --bool or --path), and match
the order of parameters with other functions (i.e. output
pointer as first parameter).

Signed-off-by: Haaris Mehmood 

---
 Documentation/git-config.txt |  5 +
 builtin/config.c | 10 +-
 builtin/reflog.c | 14 ++
 config.c | 10 ++
 config.h |  1 +
 t/helper/test-date.c | 12 
 t/t1300-repo-config.sh   | 30 ++
 7 files changed, 69 insertions(+), 13 deletions(-)

update v4: preserve the error handling style of builtin/reflog.c

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 4edd09fc6..14da5fc15 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -180,6 +180,11 @@ See also <>.
value (but you can use `git config section.variable ~/`
from the command line to let your shell do the expansion).
 
+--expiry-date::
+   `git config` will ensure that the output is converted from
+   a fixed or relative date-string to a timestamp. This option
+   has no effect when setting the value.
+
 -z::
 --null::
For all options that output values and/or keys, always
diff --git a/builtin/config.c b/builtin/config.c
index d13daeeb5..ab5f95476 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -52,6 +52,7 @@ static int show_origin;
 #define TYPE_INT (1<<1)
 #define TYPE_BOOL_OR_INT (1<<2)
 #define TYPE_PATH (1<<3)
+#define TYPE_EXPIRY_DATE (1<<4)
 
 static struct option builtin_config_options[] = {
OPT_GROUP(N_("Config file location")),
@@ -80,6 +81,7 @@ static struct option builtin_config_options[] = {
OPT_BIT(0, "int", , N_("value is decimal number"), TYPE_INT),
OPT_BIT(0, "bool-or-int", , N_("value is --bool or --int"), 
TYPE_BOOL_OR_INT),
OPT_BIT(0, "path", , N_("value is a path (file or directory 
name)"), TYPE_PATH),
+   OPT_BIT(0, "expiry-date", , N_("value is an expiry date"), 
TYPE_EXPIRY_DATE),
OPT_GROUP(N_("Other")),
OPT_BOOL('z', "null", _null, N_("terminate values with NUL byte")),
OPT_BOOL(0, "name-only", _values, N_("show variable names only")),
@@ -159,6 +161,11 @@ static int format_config(struct strbuf *buf, const char 
*key_, const char *value
return -1;
strbuf_addstr(buf, v);
free((char *)v);
+   } else if (types == TYPE_EXPIRY_DATE) {
+   timestamp_t t;
+   if (git_config_expiry_date(, key_, value_) < 0)
+   return -1;
+   strbuf_addf(buf, "%"PRItime, t);
} else if (value_) {
strbuf_addstr(buf, value_);
} else {
@@ -273,12 +280,13 @@ static char *normalize_value(const char *key, const char 
*value)
if (!value)
return NULL;
 
-   if (types == 0 || types == TYPE_PATH)
+   if (types == 0 || types == TYPE_PATH || types == TYPE_EXPIRY_DATE)
/*
 * We don't do normalization for TYPE_PATH here: If
 * the path is like ~/foobar/, we prefer to store
 * "~/foobar/" in the config file, and to expand the ~
 * when retrieving the value.
+* Also don't do normalization for expiry dates.
 */
return xstrdup(value);
if (types == TYPE_INT)
diff --git a/builtin/reflog.c b/builtin/reflog.c
index ab31a3b6a..223372531 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -416,16 +416,6 @@ static struct reflog_expire_cfg *find_cfg_ent(const char 
*pattern, size_t len)
return ent;
 }
 
-static int parse_expire_cfg_value(const char *var, const char *value, 
timestamp_t *expire)
-{
-   if (!value)
-   return config_error_nonbool(var);
-   if (parse_expiry_date(value, expire))
-   return error(_("'%s' for '%s' is not a valid timestamp"),
-value, var);
-   return 0;
-}
-
 /* expiry timer slot */
 #define EXPIRE_TOTAL   01
 #define EXPIRE_UNREACH 02
@@ -443,11 +433,11 @@ static int reflog_expire_config(const char *var, const 
char *value, void *cb)
 
if (!strcmp(key, "reflogexpire")) {
slot = EXPIRE_TOTAL;
-   if (parse_expire_cfg_value(var, 

Re: [PATCH v3 1/1] Introduce git add --renormalize .

[Junio C Hamano]

Eric Sunshine  writes:

> I _could_ understand if this functionality lived in, say, a new
> command git-attr:
> ...
> (I have since read the thread in which Junio's suggested[1] that
> git-add could house this functionality, but it still feels too
> high-level.)

If this were part of a hypothetical "git attr" command, I would have
a hard time understanding it.  I would view the "attr" as attributes
attached to each path, telling various things about the path, one of
which may be how contents are handled between Git and the file on
the filesystem.  In other words, "attr" is _not_ contents; it is a
set of attributes attached to path that house the contents.

What we are correcting here at this point in the expected usecase is
not an attribute.  We are correting the contents, and between the
two potential sources of "the right version" of the contents, the
user tells that the filesystem is the right one, so "add" is used
to correct mistakes in the index based on what is in the filesystem
(if we were correcting the other way, like Jonathan said in the
message in the thread you read, "checkout" would be used to correct
the filesystem version using what is in the index).

Re: [PATCH v2] launch_editor(): indicate that Git waits for user input

[Junio C Hamano]

Eric Sunshine  writes:

>> @@ -40,6 +40,32 @@ int launch_editor(const char *path, struct strbuf 
>> *buffer, const char *const *en
>> +   static const char *close_notice = NULL;
>> +
>> +   if (isatty(2) && !close_notice) {
>
> If you reverse this condition to say (!close_notice && isatty(2)),
> then you save an isatty() invocation each time if close_notice is
> already assigned.
>
> However, it's not clear how much benefit you gain from stashing this
> away in a static variable. Premature optimization?

The variable being "static" could be (but it was done primarily
because it allowed me not to worry about freeing), but during a
single invocation, it primarily is used as a flag "are we showing
the editor invocation notice?" two places in the function can use
without having to do the same checks twice.

If we want this as an optimization "we've already checked the
condition in our previous call, so let's use the result without
checking again", then this has to become tristate:

 - We haven't checked, and needs checking (probably NULL);

 - We have checked, and we know we want to show the notice---here is
   a string to use when we clear the notice (what we have here);

 - We have checked, and we know we do not want to show the notice
   (there is no provision for this in the posted code---that makes
   this not an optimization yet).

Perhaps an empty string can be used for the last one, but if/when it
is done, the above needs to go in a comment near the definition of
the variable.

> Should printing of close_notice be done before the error()? Otherwise,
> you get this:
>
> --- 8< ---
> Launched your editor (...) ...There was a problem...
> --- 8< ---

In my version with a far shorter message, I deliberately chose not
to clear the notice.  We ran the editor, and we saw a problem.  That
is what happened and that is what will be left on the terminal.

I agree that the verbose message Lars substituted makes it harder to
read.

Re: [PATCH 1/2] Documentation about triangular workflow

[Junio C Hamano]

Daniel Bensoussan  writes:

> +TRIANGULAR WORKFLOW
> +---
> +
> +Introduction
> +
> +
> +In some projects, contributors cannot push directly to the project but
> +have to suggest their commits to the maintainer (e.g. pull requests).
> +For these projects, it's common to use what's called a *triangular
> +workflow*:
> + ...
> +Motivations
> +~~~
> +
> +* Allows contributors to work with Git even if they don't have
> +write access to **UPSTREAM**.
> +
> +Indeed, in a centralized workflow, a contributor without write access
> +could write some code but could not send it by itself.  The contributor
> +was forced to create a mail which shows the difference between the
> +new and the old code, and then send it to a maintainer to commit
> +and push it.  This isn't convenient at all, neither for the
> +contributor, neither for the maintainer. With the triangular
> +workflow, the contributors have the write access on **PUBLISH**
> +so they don't have to pass upon maintainer(s).  And only the
> +maintainer(s) can push from **PUBLISH** to **UPSTREAM**.
> +This is called a distributed workflow (See "DISTRIBUTED WORKFLOWS"
> +above).

I probably should not be judging if these additions to
gitworkflows.txt is a good idea in the first place without seeing
any explanation as to why this patch is here, but I think it misses
the place where "triangular" sits in a larger picture.

The workflow to contrast against to illustrate the motivation is a
centralized workflow, where everybody pushes their updates to a
single place.  It does have problems inherent to its structure
(e.g. "review before integration" is much harder, if possible), and
also has its merits (e.g. it is simpler to explain and reason
about).

If you want to wean a project off of the centralized model, you'd
need to use the "distributed workflow".  The workflow to review and
apply mailed patches in public, and the workflow to have the project
pull from many publish repositories individual contributor has, are
two that allows the project to go distributed.  These two are
complementary choices with pros and cons, and it is not like one is
an improvement of the other.  Projects like the kernel even uses
hybrid of the two---the patches are reviewed in public at central
places (i.e. subsystem mailing lists) in an e-mail form and go
through iterations getting polished, and the polished results are
collected by (sub)maintainers and sent upwards, either as a request
to pull from publish repositories maintained by (sub)maintainers, or
relayed again in e-mail form (the last mile being e-mail primarily
serves as a transport vehicle for changes proven to be good, not as
material to be further reviewed).

The reason why projects make these choices is because there are pros
and cons.  A large collection of changes is far easier to integrate
with one command (i.e. "git pull") and with a need to resolve merge
conflicts just once, than applying many small changes as e-mailed
patches, having to resolve many conflicts along the way.  In order
to ensure quality of the individual changes, however, the changes
need to be reviewed and polished, and the reality of the life is
that there are far fewer people who are qualified to adequately
review and help polishing the changes than those who make changes.
Asking reviewers to go to different repositories (whose number
scales with the number of contributors) and leave comments in the
webforms is much less efficient and more costly for the project
overall, than asking them to subscribe to relevant mailing lists
(whose number scales only with the number of areas of interest) and
conduct reviews there.  Other factors like "offline access" also
count when considering the two models as "choices".

As long as the document uses phrases like "forced to", "isn't
convenient at all", etc., it is clear that it starts from a wrong
premise, "one is an improvement over the other".

[PATCH v2] rebase: use mboxrd format to avoid split errors

[Eric Wong]

Sorry, I forgot about this for a while :x

Florian Weimer  wrote:
> On 10/08/2017 11:30 PM, Eric Wong wrote:
> >diff --git a/git-rebase--am.sh b/git-rebase--am.sh
   

> My context is slightly different, but I added the mboxrd options manually to
> both commands, and it fixes my test case.
> 
> I'm still wondering if I have to be on the lookout for similar issues with
> different strings.

I don't think theres other issues with different strings,
"From " lines are the one known ambiguity problem with mbox and
thus the reason mboxrd was created.

Below is an updated patch which adds a test case:

-8<-
Subject: [PATCH] rebase: use mboxrd format to avoid split errors

The mboxrd format allows the use of embedded "From " lines in
commit messages without being misinterpreted by mailsplit

Reported-by: Florian Weimer 
Signed-off-by: Eric Wong 
---
 git-rebase--am.sh |  2 ++
 t/t3400-rebase.sh | 22 ++
 2 files changed, 24 insertions(+)

diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index 6e64d40d6f..14c50782e0 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -53,6 +53,7 @@ else
 
git format-patch -k --stdout --full-index --cherry-pick --right-only \
--src-prefix=a/ --dst-prefix=b/ --no-renames --no-cover-letter \
+   --pretty=mboxrd \
$git_format_patch_opt \
"$revisions" ${restrict_revision+^$restrict_revision} \
>"$GIT_DIR/rebased-patches"
@@ -83,6 +84,7 @@ else
fi
 
git am $git_am_opt --rebasing --resolvemsg="$resolvemsg" \
+   --patch-format=mboxrd \
$allow_rerere_autoupdate \
${gpg_sign_opt:+"$gpg_sign_opt"} <"$GIT_DIR/rebased-patches"
ret=$?
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index f5fd15e559..8ac58d5ea5 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -255,4 +255,26 @@ test_expect_success 'rebase commit with an ancient 
timestamp' '
grep "author .* 34567 +0600$" actual
 '
 
+test_expect_success 'rebase with "From " line in commit message' '
+   git checkout -b preserve-from master~1 &&
+   cat >From_.msg <
+Date: Sat, 11 Nov 2017 00:00:00 +
+Subject: not this message
+
+something
+EOF
+   >From_ &&
+   git add From_ &&
+   git commit -F From_.msg &&
+   git rebase master &&
+   git log -1 --pretty=format:%B >out &&
+   test_cmp From_.msg out
+'
+
 test_done
-- 
EW

Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file())

[Jeff King]

On Mon, Nov 06, 2017 at 05:13:15PM +0100, Simon Ruderich wrote:

> On Sat, Nov 04, 2017 at 10:07:00PM -0400, Jeff King wrote:
> > Yes, I think what you've written here (and below) is quite close to the
> > error_context patches I linked elsewhere in the thread. In other
> > words, I think it's a sane approach.
> 
> In contrast to error_context I'd like to keep all exiting
> behavior (die, ignore, etc.) in the hand of the caller and not
> use any callbacks as that makes the control flow much harder to
> follow.

Yeah, I have mixed feelings on that. I think it does make the control
flow less clear. At the same time, what I found was that handlers like
die/ignore/warn were the thing that gave the most reduction in
complexity in the callers.

> Regarding the API, should it be allowed to pass NULL as error
> pointer to request no additional error handling or should the
> error functions panic on NULL? Allowing NULL makes partial
> conversions possible (e.g. for write_in_full) where old callers
> just pass NULL and check the return values and converted callers
> can use the error struct.

I think it's probably better to be explicit, and pass some "noop" error
handling struct. We'll have to be adding parameters to functions to
handle this anyway, so I don't think there's much opportunity for having
NULL as a fallback for partial conversions.

> How should translations get handled? Appending ": %s" for
> strerror(errno) might be problematic. Same goes for "outer
> message: inner message" where the helper function just inserts ":
> " between the messages. Is _("%s: %s") (with appropriate
> translator comments) enough to handle these cases?

I don't have a real opinion, not having done much translation myself. I
will say that the existing die_errno(), error_errno(), etc just use "%s:
%s", without even allowing for translation (see fmt_with_err in
usage.c). I'm sure that probably sucks for RTL languages, but I think
it would be fine to punt on it for now.

> Suggestions how to name the struct and the corresponding
> functions? My initial idea was struct error and to use error_ as
> prefix, but I'm not sure if struct error is too broad and may
> introduce conflicts with system headers. Also error_ is a little
> long and could be shorted to just err_ but I don't know if that's
> clear enough. The error_ prefix doesn't conflict with many git
> functions, but there are some in usage.c (error_errno, error,
> error_routine).

In my experiments[1] I called the types error_*, and then generally used
"err" as a local variable when necessary. Variants on that seem fine to
me, but yeah, you have to avoid conflicting with error(). We _could_
rename that, but it would be a pretty invasive patch.

> And as general question, is this approach to error handling
> something we should pursue or are there objections? If there's
> consensus that this might be a good idea I'll look into
> converting some parts of the git code (maybe refs.c) to see how
> it pans out.

I dunno. I kind of like the idea, but if the only error context is one
that adds to strbufs, I don't know that it's buying us much over the
status quo (which is passing around strbufs). It's a little more
explicit, I guess.

Other list regulars besides me seem mostly quiet on the subject.

-Peff

[1] This is the jk/error-context-wip branch of https://github.com/peff/git.
I can't remember if I mentioned that before.

Re: [PATCH v4 4/6] list-objects: filter objects in traverse_commit_list

[Jeff King]

On Fri, Nov 17, 2017 at 10:42:52AM -0500, Jeff Hostetler wrote:

> > Yes, I share the same feeling.  It does not help that the series
> > defines its own notion of arg_needs_armor() and uses it to set a
> > field called requires_armor that is not yet used, the definition of
> > "armor"ing being each byte getting encoded as two hexadecimal digits
> > without any sign (which makes me wonder what a receiver of
> > "deadbeef" would do---did it receive an armored string or a plain
> > one???).  I do not understand why these strings are not passed as
> > opaque sequences of bytes and instead converted at this low a layer.
> 
> I'm probably being too paranoid.  My fear is that a client could pass
> an expression to clone/fetch/fetch-pack that would be sent to the
> server and evaluated by the interface between upload-pack and pack-objects.
> I'm not worried about the pack-protocol transport.  I'm mainly concerned
> in how upload-pack passes that *client-expression* to pack-objects and are
> there ways for that to go south on the server with a carefully crafted
> expression.

I think you have to trust that those interfaces are capable of passing
raw bytes, whether directly via execve() or because we got the quoting
right. If there's a bug there, it's going to be a bigger problem than
just this code path (and the fix needs to be there, not second-guessing
it in the callers).

So I'd say that yeah, you are being too paranoid.

As an aside, though, I wonder if these client expressions should be fed
over stdin to pack-objects. That removes any argv limits we might run
into on the server side. It also removes shell injections as a
possibility, though of course we'd need quoting in that layer to avoid
argument-injection to pack-objects.

-Peff

Re: [PATCH] Reduce performance penalty for turned off traces

[Jeff King]

On Wed, Nov 15, 2017 at 11:14:20AM -0800, Stefan Beller wrote:

> > I did manually disable HAVE_VARIADIC_MACROS and confirmed that the
> > result builds and passes the test suite (though I suspect that GIT_TRACE
> > is not well exercised by the suite).
> 
> GIT_TRACE is exercised in the test suite (though I am not sure if it counts
> as well-exercised) in t7406-submodule-update.sh for example, which uses
> GIT_TRACE to obtain information about thread parallelism used by Git, as
> that is not observable otherwise, if we assume that performance tests in the
> standard test suite are not feasible.

Hmm, yeah, that might cover it. What I'm worried about is that we missed
some case where NULL needed to become _default_key. But I did look
for that in my review of the patch and didn't notice any spots. And the
coverage in t7406 should help.

> > After your patch, the GIT_TRACE=1 time remains the same but GIT_TRACE=0
> > drops to 1ms.
> 
> So does that mean we can use a lot more tracing now?

Yep, that's the intent.

-Peff

Re: [PATCH] Reduce performance penalty for turned off traces

[Jeff King]

On Sun, Nov 12, 2017 at 11:24:11PM +, Gennady Kupava wrote:

> In reality, gcc didn't do that and I saw 3 function calls. I am pretty
> sure that compiler of the distant bright future will do that, and only
> problem would be to eliminate that single function call. Hopefully
> with -flto it will also eliminate this single functions call, at it
> will be able to see through translation units. Given that I actually
> like current implementation as it hides all details in .c file.

Yeah, I agree with your analysis that we are really just overriding what
could eventually be figured out by the compiler during LTO.

I do think this is worth pursuing in the meantime, though, because it's
not _too_ much work, and we don't know when that magical optimizing
compiler will appear. :) So this frees us up in the meantime to worry
less about the cost of tracing.

> Now, implementation you suggesting moves extra things into .h, so it
> is imperfect in terms above, while things I suggested moves only
> necessary bit of checking the necessity to do anything, which is only
> (interesting) part should be executed while traces are off, and the
> only part we really want to be inlined.

I only meant to suggest moving the necessary checking into the .h file.
It's just that we have to make it a static inline for cases where we
don't have variadic macros. Anyway, I'm OK with your original notion of
leaving the non-variadic-macro systems on the "slow" path, so we can
just go with the pure-macro thing you have.

> > So it is measurable, and we might expect that tracing a really big loop
> > (say over all of the objects in the repository) would benefit. OTOH, a
> > real loop would actually be doing other stuff, so the speedup would be
> > less dramatic. Still, it seems like an easy win.
> 
> I actually quite familiar with performance analysis and know how to
> use perf and similar tools a bit -
> so if there is something more measurable in TODO, I could try. But I
> guess this is interesting to many devs so such tasks probably already
> all done and bottlenecks are identified in git =)

There are lots of bottlenecks still to be found, I'm sure. If you want
to have a go at finding some with perf, be my guest. :)

-Peff

Re: [PATCH] config: avoid "write_in_full(fd, buf, len) != len" pattern

[Jeff King]

On Wed, Nov 15, 2017 at 12:40:43PM +, Phillip Wood wrote:

> From: Phillip Wood 
> 
> As explained in commit 06f46f237 (avoid "write_in_full(fd, buf, len)
> != len" pattern, 2017–09–13) the return value of write_in_full() is
> either -1 or the requested number of bytes. As such comparing the
> return value to an unsigned value such as strbuf.len will fail to
> catch errors. Change the code to use the preferred '< 0' check.

Thanks for catching this. I wondered at first how I missed these obvious
cases, but the answer is that they were added after my commit. :)

There's one more case in write_section() that uses "==". That's not
actually wrong, but I wonder if we'd want to make it "< 0" for
consistency.

-Peff

Re: Bug in "revision.c: --all adds HEAD from all worktrees" ?

[Jeff King]

On Wed, Nov 15, 2017 at 09:38:26PM +, Luke Diamand wrote:

> >> Quite a few of the worktrees have expired - their head revision has
> >> been GC'd and no longer points to anything sensible
> >> (gc.worktreePruneExpire). The function other_head_refs() in worktree.c
> >> bails out if there's an error, which I think is the problem. I wonder
> >> if it should instead just report something and then keep going.
> >
> > Also see
> > https://public-inbox.org/git/cagz79kyp0z1g_h3nwfmshrawhmbocik5lepuxkj0nveebri...@mail.gmail.com/
> 
> So is this a bug or user error on my part?

It's a bug. You didn't do anything wrong, but the worktree feature
corrupted your repository.

-Peff

Re: [PATCH] Makefile: check that tcl/tk is installed

[Jeff King]

On Fri, Nov 17, 2017 at 12:42:58PM -0500, Todd Zullinger wrote:

> > I'd rather add a separate check for msgfmt than mixing the 2 issues,
> > because I think that unless it has been explicitly told to do so, Git
> > should not try to build git-gui and gitk in the first place if there is
> > a big chance that those tools will not work.
> 
> If that's a motivation, wouldn't a check in the gitk and git-gui scripts
> handle it?  That would provide an error at run time to the user.  This
> change is about helping the user who builds their own git and then runs it,
> so if they built git without wish installed and then ran git-gui, they'd get
> a clear error that wish is missing and could easily install it.  It's not
> needed for the build, so they wouldn't need to rebuild anything.

I think the message is already OK:

  $ ./gitk
  ./gitk: 3: exec: wish: not found

The question is whether we would want to catch this at build time. And I
think Junio's point is that we don't _know_ it's an error at build time.
We could be building gitk for use on a system that isn't quite like the
build system, so any "solution" here is going to have to make an
assumption either way.

It's also not foolproof. You could build when wish is present, and then
later uninstall it and receive the same error message.

I also think all of this is largely orthogonal to gettext. It just so
happens that if you don't have gettext installed, we'll try to run wish
as part of the build process, but detecting broken tcl setups was
definitely not part of the intent there.

And the failure actually runs the other way, too. If you have neither
gettext nor tcl, you get this confusing output:

  $ make NO_GETTEXT=1
  ...
  MSGFMT po/pt_pt.msg MSGFMTpo/hu.msg Makefile:252: recipe for target 
'po/pt_pt.msg' failed
  make[1]: *** [po/pt_pt.msg] Error 127

(the problem is not msgfmt, but our tcl substitute which cannot run).

I'm actually tempted to say that we should not be building the tcl parts
by default. IOW, instead of NO_TCLTK we should have USE_TCLTK. That
would also require an adjustment by package builders, but it would
hopefully be a really obvious one. And once the user has told our
Makefile that they definitely want to build the tcl parts, we'd
presumably just trust that the tcl path they give us is sane.

But it's possible I'm underestimating how many people actually use the
tcl scripts. Certainly I don't, and git-gui seems fairly primitive to me
these days compared to 3rd party tools. But then I don't use any of them
either. ;)

-Peff

Re: [PATCH 2/2] Triangular workflow

[Martin Ågren]

On 17 November 2017 at 17:07, Daniel Bensoussan
 wrote:
> The documentation about triangular workflow was not clear enough.

I think you would be able to `git rebase -i` these two patches into a
single, perfect patch. ;-) Maybe in collaboration with Albertin?

> -===
> -`git config push.default current`
> -===
> +
> +`git clone `
> +`git remote add **PUBLISH**`
> +`git push`
> +

This renders as a single line for me, including "**PUBLISH**". (I use
AsciiDoctor, does this look good with AsciiDoc, I wonder?)

I've never worked with triangular workflows. That means I can't judge
the correctness of this, but what I've read seems reasonable and
helpful. I dropped some comments along the way, I hope you'll find them
constructive.

Martin

Re: [PATCH 1/2] Documentation about triangular workflow

[Martin Ågren]

On 17 November 2017 at 17:07, Daniel Bensoussan
 wrote:
> +- If the maintainer accepts the changes, he merges them into the
> +  **UPSTREAM** repository.

Personally, I would prefer "they" and "their" over "he" and "his". I'm
not sure how universal this preference is, but see also 715a51bcaf (am:
counteract gender bias, 2016-07-08). I realize that "he" is already used
in this document...

> + ... The contributor
> +was forced to create a mail which shows the difference between the
> +new and the old code, and then send it to a maintainer to commit
> +and push it.  This isn't convenient at all, neither for the
> +contributor, neither for the maintainer.

"neither ... nor". That said, I find the tone of this paragraph a bit
value-loaded ("forced ... isn't convenient at all"). It does sort of
contradict or at least compare interestingly with how git.git itself is
maintained. ;-) Maybe this could be framed in a more neutral way?

> +The goal of the triangular workflow is also that the rest of the
> +community or the company can review the code before it's in production.
> +Everyone can read on **PUBLISH** so everyone can review code
> +before the maintainer(s) merge it to **UPSTREAM**.  It also means

I think you can drop the "(s)". Your example workflow could have a
single maintainer. In a multi-maintainer workflow, the workflow would
still be the same. So no need to cover all bases by sprinkling "(s)" on
the text. (IMHO.)

I'll follow up with some comments on patch 2/2...

Martin

Re: [PATCH v4 07/10] introduce fetch-object: fetch one promisor object

[Stefan Beller]

On Fri, Nov 17, 2017 at 11:49 AM, Jeff Hostetler  wrote:
>
>
> On 11/16/2017 2:57 PM, Ramsay Jones wrote:
>>
>>
>>
>> On 16/11/17 18:12, Jeff Hostetler wrote:
>>>
>>> From: Jonathan Tan 
>>>
>>> Introduce fetch-object, providing the ability to fetch one object from a
>>> promisor remote.
>
> [snip]
>>>
>>> +#include "transport.h"
>>
>>
>> I note that this still does not #include "fetch_object.h".
>> [If you recall, this suppresses a sparse warning].
>>
>
> Sorry, I missed that.  I know I did a DEVELOPER=1 build and
> I didn't see a warning, but I'll check again.

sparse is an extra tool, which you have to run/install;
it is not included in the developer build.

https://en.wikipedia.org/wiki/Sparse

Re: [PATCH v3 1/1] Introduce git add --renormalize .

[Eric Sunshine]

On Thu, Nov 16, 2017 at 11:38 AM,   wrote:
> Make it safer to normalize the line endings in a repository:
> Files that had been commited with CRLF will be commited with LF.
>
> The old way to normalize a repo was like this:
>  # Make sure that there are not untracked files
>  $ echo "* text=auto" >.gitattributes
>  $ git read-tree --empty
>  $ git add .
>  $ git commit -m "Introduce end-of-line normalization"
>
> The user must make sure that there are no untracked files,
> otherwise they would have been added and tracked from now on.
>
> The new "add ..renormalize" does not add untracked files:
>  $ echo "* text=auto" >.gitattributes
>  $ git add --renormalize .
>  $ git commit -m "Introduce end-of-line normalization"
>
> Note that "git add --renormalize " is the short form for
> "git add -u --renormalize ".
>
> While add it, document that the same renormalization may be needed,
> whenever a clean filter is added or changed.

Forgive me for chiming in so late, but as a newcomer to this topic,
the high-level choice made by this patch feels a bit questionable. I
understand that, for people familiar with the "old way" of normalizing
files, git-add might seems like the right place to house this
functionality (and perhaps that's true from an implementation angle?),
but as one coming to this topic with no existing bias about
implementation or the "old way", git-add feels like an odd choice.
This sort of normalization (emptying the index, potentially modifying
files, repopulating the index) seems too high-level for git-add.

I _could_ understand if this functionality lived in, say, a new
command git-attr:

SYNOPSIS

git attr renormalize [--no-commit | [-m ]] pathname...
git attr check [-a | --all | attr…] [--] pathname…
git attr check --stdin [-z] [-a | --all | attr…]

DESCRIPTION

'git attr renormalize'

Apply the "clean" process ... and commit the results. This is
useful after changing `core.autocrlf` ... etc. With '-m', uses
 as the commit message, else launches the editor. Use
'--no-commit' to skip the automatic commit.

The 'git attr check' command subsumes the role of existing
git-check-attr. One could envision git-attr growing additional
subcommands to edit .gitattributes, much like git-config edits
.gitconfig.

(I have since read the thread in which Junio's suggested[1] that
git-add could house this functionality, but it still feels too
high-level.)

More below...

> Signed-off-by: Torsten Bögershausen 
> ---
> diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
> @@ -175,6 +175,13 @@ for "git add --no-all ...", i.e. ignored 
> removed files.
> +--renormalize::
> +   Apply the "clean" process freshly to all tracked files to

This is the only time "clean" appears in git-add documentation. Every
newcomer to git learns about git-add very early on, but "clean
process" is a fairly advanced topic, unlikely to be on a newcomer's
radar. The term "renormalize" also feels out of place in git-add
documentation. If I was a newcomer reading git-add documentation, I
think I'd be left pretty well clueless by this description. At the
very least, perhaps add links to git-attributes and 'core.autocflf'
configuration.

> +   forcibly add them again to the index.  This is useful after
> +   changing `core.autocrlf` configuration or the `text` attribute
> +   in order to correct files added with wrong CRLF/LF line endings.
> +   This option implies `-u`.

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

Re: [PATCH v4 07/10] introduce fetch-object: fetch one promisor object

[Ramsay Jones]

On 17/11/17 19:49, Jeff Hostetler wrote:
> 
> 
> On 11/16/2017 2:57 PM, Ramsay Jones wrote:
>>
>>
>> On 16/11/17 18:12, Jeff Hostetler wrote:
>>> From: Jonathan Tan 
>>>
>>> Introduce fetch-object, providing the ability to fetch one object from a
>>> promisor remote.
> [snip]
>>> +#include "transport.h"
>>
>> I note that this still does not #include "fetch_object.h".
>> [If you recall, this suppresses a sparse warning].
>>
> 
> Sorry, I missed that.  I know I did a DEVELOPER=1 build and
> I didn't see a warning, but I'll check again.

Unless you have sparse installed and 'make sparse' as part of
your build, you won't see anything. ;-)

However, ignore sparse for the moment, if you don't #include
the header (interface) file in fetch-object.c, you can't
expect the compiler to tell you when there is a mismatch
between the interface and implementation.

There are some additional sparse warnings associated with
these series, ... (hopefully I can find some time tonight)

ATB,
Ramsay Jones

Re: [PATCH 1/2] Documentation about triangular workflow

[Stefan Beller]

Thanks for contributing to Git and making the documentation better!

On Fri, Nov 17, 2017 at 8:07 AM, Daniel Bensoussan
 wrote:
> From: ALBERTIN TIMOTHEE 11514771 

This is a place where you can describe why this change is awesome (and
hence the project is interested in including it.)

The Git projects requires your Sign-off (and by the differing author
and you as a sender, both of you)
See Documentation/SubmittingPatches or
https://developercertificate.org/ and if you can agree
to that add a line "Signed-off-by: name " to the commit message
of the text.

>
> diff --git a/Documentation/gitworkflows.txt b/Documentation/gitworkflows.txt
> index 02569d061..3f1ddba82 100644
> --- a/Documentation/gitworkflows.txt
> +++ b/Documentation/gitworkflows.txt
> @@ -464,6 +464,221 @@ in patches to figure out the merge base.  See 
> linkgit:git-am[1] for
>  other options.
>
>
> +TRIANGULAR WORKFLOW
> +---
> +
> +Introduction
> +
> +
> +In some projects, contributors cannot push directly to the project but
> +have to suggest their commits to the maintainer (e.g. pull requests).
> +For these projects, it's common to use what's called a *triangular
> +workflow*:
> +
> +- The project maintainer publishes a repository, called **UPSTREAM** in
> +this document, which is a read-only for contributors. They can clone and
> +fetch from this repository.
> +- Contributors publish their modifications by pushing to a repository,
> +called **PUBLISH** in this document, and request a merge.
> +- Opening a pull request
> +- If the maintainer accepts the changes, he merges them into the
> +  **UPSTREAM** repository.
> +
> +This workflow is commonly used on different platforms like BitBucket,
> +GitHub or GitLab which provide a dedicated mechanism for requesting merges.
> +
> +
> +--   -
> +| UPSTREAM   |  maintainer   | PUBLISH   |
> +|  git/git   |- - - - - - - -|  me/remote|
> +--  <-   -
> +  \ /
> +   \   /
> +fetch | \ / ^ push
> +  v  \   /  |
> +  \ /
> +   -
> +   |   LOCAL   |
> +   -
> +

git/git as the upstream is a notable example which doesn't
use this triangular workflow, as most patches are accepted
via email, such that the PUBLISH remote may not exist when
contributing to git/git. (Though https://submitgit.herokuapp.com/
tries to emulate the triangular workflow for contributors)


> +This is just a side-effect of the "review before merge" mentionned
> +above but this is still a good point.

mentioned (typo)

Re: [PATCH v4 07/10] introduce fetch-object: fetch one promisor object

[Jeff Hostetler]

On 11/16/2017 2:57 PM, Ramsay Jones wrote:



On 16/11/17 18:12, Jeff Hostetler wrote:

From: Jonathan Tan 

Introduce fetch-object, providing the ability to fetch one object from a
promisor remote.

[snip]

+#include "transport.h"


I note that this still does not #include "fetch_object.h".
[If you recall, this suppresses a sparse warning].



Sorry, I missed that.  I know I did a DEVELOPER=1 build and
I didn't see a warning, but I'll check again.

Thanks,
Jeff

Re: [PATCH v2] launch_editor(): indicate that Git waits for user input

[Eric Sunshine]

On Fri, Nov 17, 2017 at 8:51 AM,   wrote:
> When a graphical GIT_EDITOR is spawned by a Git command that opens
> and waits for user input (e.g. "git rebase -i"), then the editor window
> might be obscured by other windows. The user may be left staring at the
> original Git terminal window without even realizing that s/he needs to
> interact with another window before Git can proceed. To this user Git
> appears hanging.
>
> Show a message in the original terminal and get rid of it when the
> editor returns.
>
> Signed-off-by: Junio C Hamano 
> Signed-off-by: Lars Schneider 
> ---
> diff --git a/editor.c b/editor.c
> @@ -40,6 +40,32 @@ int launch_editor(const char *path, struct strbuf *buffer, 
> const char *const *en
> +   static const char *close_notice = NULL;
> +
> +   if (isatty(2) && !close_notice) {

If you reverse this condition to say (!close_notice && isatty(2)),
then you save an isatty() invocation each time if close_notice is
already assigned.

However, it's not clear how much benefit you gain from stashing this
away in a static variable. Premature optimization?

> +   char *term = getenv("TERM");
> +
> +   if (term && strcmp(term, "dumb"))
> +   /*
> +* go back to the beginning and erase the
> +* entire line if the terminal is capable
> +* to do so, to avoid wasting the vertical
> +* space.
> +*/
> +   close_notice = "\r\033[K";
> +   else
> +   /* otherwise, complete and waste the line */
> +   close_notice = "done.\n";
> +   }
> +
> +   if (close_notice) {
> +   fprintf(
> +   stderr,
> +   "Launched your editor ('%s'). Adjust, save, 
> and close the "
> +   "file to continue. Waiting for your input... 
> ", editor
> +   );

Here's what this looks like for me:

--- 8< ---
Launched your editor
('/Applications/Emacs.app/Contents/MacOS/bin/emacsclient'). Adjust,
save, and close the file to continue. Waiting for your input...
Waiting for Emacs...
--- 8< ---

Very, very noisy, so much so that it's almost unreadable. There are at
least three reasons for the noise:

* The raw message itself is already overly long. Do we really need to
assume that newcomers are so clueless that they need it spelled out to
such a level of detail? "Launched editor" should be enough for most
people, and one would hope that "Launched editor; waiting for
input..." would be enough for the rest.

* Does not take into consideration that EDITOR might be very long;
perhaps you could just print the basename and strip arguments (i.e.
"/my/long/path/edit -x --foo --zap" becomes "edit"). Or, just omit the
editor altogether.

* emacsclient already prints its own message ("Waiting for Emacs...",
which runs together with Git's message). Perhaps treat emacsclient as
a special case and skip printing this message if emacsclient is in
use: if (strstr(...,"emacsclient"))

And, of course, with a "dumb" terminal, it's even noisier with the
extra "done." at the end:

--- 8< ---
Launched your editor
('/Applications/Emacs.app/Contents/MacOS/bin/emacsclient'). Adjust,
save, and close the file to continue. Waiting for your input...
Waiting for Emacs...
done.
--- 8< ---

As Junio pointed out in [1], emacsclient has already emitted a
newline, so the clear-line sequence is ineffective; likewise, for a
dumb terminal, "done." ends up on its own line. Aside from the noise,
this also suggests making a special case for emacsclient.

And, as Junio pointed out in [2], with a message so long, once it has
wrapped, the clear-line sequence does not work as intended. For those
of us with 80-column terminals, we're left with a bunch of noise on
the screen.

> +   fflush(stderr);
> +   }
>
> p.argv = args;
> p.env = env;
> @@ -53,11 +79,14 @@ int launch_editor(const char *path, struct strbuf 
> *buffer, const char *const *en
> sig = ret - 128;
> sigchain_pop(SIGINT);
> sigchain_pop(SIGQUIT);
> +
> if (sig == SIGINT || sig == SIGQUIT)
> raise(sig);
> if (ret)
> return error("There was a problem with the editor 
> '%s'.",
> editor);
> +   if (close_notice)
> +   fputs(close_notice, stderr);

Should printing of close_notice be done before the error()? Otherwise,
you get this:

--- 8< ---
Launched your editor (...) ...There was a problem...

Re: [PATCH v2] launch_editor(): indicate that Git waits for user input

[Junio C Hamano]

lars.schnei...@autodesk.com writes:

> Junio posted the original version of this patch [1] as response to my RFC [2].
> I took Junio's patch and slightly changed the commit message as well as the
> message printed to the user after GIT_EDITOR is invoked [3].
>
> Thanks,
> Lars

Thanks.

> diff --git a/editor.c b/editor.c
> index 7519edecdc..23db92d8c6 100644
> --- a/editor.c
> +++ b/editor.c
> @@ -40,6 +40,32 @@ int launch_editor(const char *path, struct strbuf...
> ...
> + if (close_notice) {
> + fprintf(
> + stderr,
> + "Launched your editor ('%s'). Adjust, save, and 
> close the "
> + "file to continue. Waiting for your input... ", 
> editor

How wide is your typical terminal window?  With message this long, a
sample standalone program I used while developing the prototype of
this feature no longer can retract this "temporary" message.

Would something shorter like "Waiting for you to finish editing..."
work well enough?

-- -- --
#include 

int main(void)
{
const char *EL = "\033[K"; /* Erase in Line */
const char *editor = "emacsclient";

fprintf(
stderr,
"Launched your editor ('%s'). Adjust, save, and close the "
"file to continue. Waiting for your input... ", editor);
fflush(stderr);
sleep(2);
fprintf(stderr, "\r%s", EL);
fflush(stderr);
return 0;
}
-- -- --

Re: [PATCH V3] config: add --expiry-date

[hsed]

On 2017-11-16 00:54, Junio C Hamano wrote:



-   if (parse_expiry_date(value, expire))
-   return error(_("'%s' for '%s' is not a valid timestamp"),
-value, var);
...
+   if (parse_expiry_date(value, timestamp))
+   die(_("failed to parse date_string in: '%s'"), value);


This is an unintended change in behaviour (or at least undocumented
in the log message) for the "git reflog" command, no?

Not just the error message is different, but the original gave the
calling code a chance to react to the failure by returning -1 from
the function, but this makes the command fail outright here.

Would it break anything if you did "return error()" just like the
original used to?  Are your callers of this new function not
prepared to see an error return?


I did notice the slight change in the error handling but the new one
was copied from one of the other functions in builtin/config.c. I
will revert it to the old method and if the new (and other) tests still
pass, I will provide an updated patch.

Kind Regards,
Haaris

Re: [PATCH] Makefile: check that tcl/tk is installed

[Todd Zullinger]

Christian Couder wrote:

On Thu, Nov 16, 2017 at 2:35 AM, Junio C Hamano  wrote:
I suspect that this change will hurt those who package Git for other 
people.


Maybe a little bit, but in my opinion it should not be a big problem 
for them to install Tcl/Tk and its dependencies on the build machine.


It's not a big burden, but it is a seemingly unnecessary build-time 
dependency.


It used to be that, as long as they have msgfmt installed, they only 
needed to _know_ what the path on the users' box to "wish" is, and 
set it to TCLTK_PATH, and if they are distro packagers, most likely 
they already have such an automated set-up working.  Now with this 
change, they are forced to install tcl/tk on their possibly headless 
box where tcl/tk is useless, and worse yet, an attempt to install it 
may bring in tons of unwanted stuff related to X that is irrelevant 
on such a headless development environment.


Yeah, but if they build gitk and git-gui, there is a significant 
chance that they build other graphical software too, and that this 
will require installing stuff related to X anyway.


Most distributions build packages in individual container or chroots, 
to increase the stability and reproducibility of the builds.  So 
package builds don't run on systems where any deps have already been 
installed.


To be fair, it looks like pulling in tcl/tk would add only around 8MB 
to the Fedora build root for git.  That's not egregious, to be sure.  
But it really isn't a necessary build-time dependency either.  I don't 
know if there are other distros who would strongly object to pulling 
in tcl/tk.  Some are much more sensitive to build root sizes and 
unnecessary dependencies.


In general I think packagers are much more able to deal with those 
kinds of problems than most regular developers who want to hack on 
Git.


I agree.  Packagers also provide git builds to the vast majority of 
end-users, so we should make their task easier whenever possible. :)


So asking packagers to either set NO_TCLTK or BYPASS_TCLTK_CHECK or to 
install Tcl/Tk would not burden them much, especially compared to what 
regular developers have to deal with these days when trying to build 
Git.


Presuming this new BYPASS_TCLTK_CHECK is communicated well and that 
the failure when not using it is clear, this doesn't seem likely to 
cause problems.  (I'll leave it to others whether there's a better way 
to solve the msgfmt fallthrough issue.  I didn't even know such a 
fallthrough existed until yesterday.)


I think it's important to ensure that automated package builds of a 
newer git don't simply skip parts of the build which used to work and 
so packagers reading the failed builds logs can easily see what they 
need to adjust.


Just dropping the new variable in the Makefile and waiting for package 
builds to fail or not package gitk & git-gui at the next release would 
be a bit unkind, I think.  Posting this to the git-packagers group[1] 
which Ævar created would be useful.  It /might/ even be worth asking 
there if any distros have strong opinions on the subject.


[1] https://groups.google.com/forum/#!forum/git-packagers and
   git-packag...@googlegroups.com

I think "If I cannot run either wish or msgfmt, then barf and give 
an error message" might at least be needed.  Am I misinterpreting 
the motivation of the patch?


I'd rather add a separate check for msgfmt than mixing the 2 issues, 
because I think that unless it has been explicitly told to do so, 
Git should not try to build git-gui and gitk in the first place if 
there is a big chance that those tools will not work.


If that's a motivation, wouldn't a check in the gitk and git-gui 
scripts handle it?  That would provide an error at run time to the 
user.  This change is about helping the user who builds their own git 
and then runs it, so if they built git without wish installed and then 
ran git-gui, they'd get a clear error that wish is missing and could 
easily install it.  It's not needed for the build, so they wouldn't 
need to rebuild anything.


Something like this:

diff --git i/gitk-git/gitk w/gitk-git/gitk
index a14d7a16b2..f9f28a164a 100755
--- i/gitk-git/gitk
+++ w/gitk-git/gitk
@@ -1,5 +1,6 @@
#!/bin/sh
-# Tcl ignores the next line -*- tcl -*- \
+# Tcl ignores the next 2 lines -*- tcl -*- \
+type wish >/dev/null 2>&1 || { echo "error: wish not found" >&2; exit 1; }; \
exec wish "$0" -- "$@"

# Copyright © 2005-2016 Paul Mackerras.  All rights reserved.

maybe?  (The error message is certainly open for improvement.)

--
Todd
~~
A fool's brain digests philosophy into folly, science into
superstition, and art into pedantry.  Hence University education.
   -- George Bernard Shaw

Re: [PATCH v4 5/6] rev-list: add list-objects filtering support

[Jeff Hostetler]

On 11/16/2017 9:14 PM, Junio C Hamano wrote:

Jonathan Tan  writes:


If it were up to me, I would remove all existing mentions of "partial
clone" and explain the presence of the "--missing" argument as follows:

 In the future, we will introduce a "partial clone" mechanism wherein
 an object in a repo, obtained from a remote, may reference a missing
 object that can be dynamically fetched from that remote once needed.
 This "partial clone" mechanism will have a way, sometimes slow, of
 determining if a missing link is one of the links expected to be
 produced by this mechanism.

 This patch introduces handling of missing objects to help debugging
 and development of the "partial clone" mechanism, and once the
 mechanism is implemented, for a power user to perform operations
 that are missing-object-aware without incurring the cost of checking
 if a missing link is expected.


That sounds quite sensible.



will do.  thanks.
Jeff

Re: is there a rationale for some sample hooks but not others?

[Jonathan Nieder]

Hi Robert,

Robert P. J. Day wrote:

> given that a newly-initialized repo contains samples for some hooks
> but not others, is there a simple rationale for why those particular
> sample hooks are provided, and not the rest?

I assume this is in the context of reviewing the Pro Git book.  Thanks
for doing this work.

Let me turn the question around: do you think it would be useful to
have samples for all hook types?  What would you like those samples to
look like?

Thanks,
Jonathan

[PATCH 2/2] Triangular workflow

[Daniel Bensoussan]

The documentation about triangular workflow was not clear enough.
So it couldn't be clear enough for someone else. That's why we decided to
change it. Some documentation about triangular workflow already exists.
However, it seems unfortunate that there is no documentation about it in Git.

Based-on-patch-by: Jordan DE GEA 
Signed-off-by: Michael Haggerty 
Signed-off-by: Jordan DE GEA 
Signed-off-by: Matthieu Moy 
Signed-off-by: Timothee Albertin 
Signed-off-by: Nathan Payre 
Signed-off-by: Daniel Bensoussan 
---
 Documentation/gitworkflows.txt | 59 +++---
 1 file changed, 26 insertions(+), 33 deletions(-)

diff --git a/Documentation/gitworkflows.txt b/Documentation/gitworkflows.txt
index 3f1ddba82..4aea16bc0 100644
--- a/Documentation/gitworkflows.txt
+++ b/Documentation/gitworkflows.txt
@@ -490,7 +490,7 @@ GitHub or GitLab which provide a dedicated mechanism for 
requesting merges.
 
 --   -
 | UPSTREAM   |  maintainer   | PUBLISH   |
-|  git/git   |- - - - - - - -|  me/remote|
+||- - - - - - - -|   |
 --  <-   -
   \ /
\   /
@@ -515,7 +515,7 @@ new and the old code, and then send it to a maintainer to 
commit
 and push it.  This isn't convenient at all, neither for the
 contributor, neither for the maintainer. With the triangular
 workflow, the contributors have the write access on **PUBLISH**
-so they don't have to pass upon maintainer(s).  And only the
+so they don't need maintainer(s) approval to write code.  And only the
 maintainer(s) can push from **PUBLISH** to **UPSTREAM**.
 This is called a distributed workflow (See "DISTRIBUTED WORKFLOWS"
 above).
@@ -526,13 +526,13 @@ The goal of the triangular workflow is also that the rest 
of the
 community or the company can review the code before it's in production.
 Everyone can read on **PUBLISH** so everyone can review code
 before the maintainer(s) merge it to **UPSTREAM**.  It also means
-that, in a free software, anyone can propose code without danger
+that, in free software, anyone can propose code without danger
 for the stability of the software.
 
 * Encourages clean history by using `rebase -i` and `push --force` to
 the public fork before the code is merged.
 
-This is just a side-effect of the "review before merge" mentionned
+This is just a side-effect of the "review before merge" mentioned
 above but this is still a good point.
 
 
@@ -543,18 +543,20 @@ workflow.
 Preparation as a contributor
 
 
-Cloning from **PUBLISH**, which is a fork of **UPSTREAM** or an empty
-repository.
+Cloning from **UPSTREAM**.
 
 ==
-`git clone `
+`git clone `
 ==
 
-Setting the behavior of push for the triangular workflow:
+If **PUBLISH** doesn't exist, a contributor can publish his own repository.
+**PUBLISH** contains modifications before integration.
 
-===
-`git config push.default current`
-===
+
+`git clone `
+`git remote add **PUBLISH**`
+`git push`
+
 
 Adding **UPSTREAM** remote:
 
@@ -576,12 +578,6 @@ Example with master as :
 * `git config branch.master.pushRemote origin`
 ===
 
-Staying up-to-date
-~~
-
-Retrieve updates from **UPSTREAM** with `git pull` and send them to
-**PUBLISH** with `git push`.
-
 Making your work available
 ~~
 
@@ -590,20 +586,19 @@ the **UPSTREAM** thanks to the configuration you did 
earlier with the
 `git config remote.pushdefault origin` command.
 
 When a contributor pushes something, the `git config push.default
-current` command can be used to specifies that the name of the
+current` command can be used to specify that the name of the
 **PUBLISH** branch is the same as the name of the **LOCAL** one.
 
-.Display the push remote's name:
+.Display the name of the push remote:
 [caption="Recipe: "]
 
-=
-`git rev-parse --abbrev-ref @{push}`
-=
-
 The shorthand `@{push}` denotes the remote-tracking branch
 where the  would be pushed to. If no  is specified
 (`@{push}`),  takes the value of the current branch.
 
+=
+`git rev-parse --abbrev-ref @{push}`
+=
 
 .Display the fetch remote's name:
 [caption="Recipe: "]
@@ -630,6 +625,12 @@ takes the value of the current branch.
 `git log @{push}..`
 
 
+Staying up-to-date
+~~
+

[PATCH 1/2] Documentation about triangular workflow

[Daniel Bensoussan]

From: ALBERTIN TIMOTHEE 11514771 

---
 Documentation/gitworkflows.txt | 215 +
 1 file changed, 215 insertions(+)

diff --git a/Documentation/gitworkflows.txt b/Documentation/gitworkflows.txt
index 02569d061..3f1ddba82 100644
--- a/Documentation/gitworkflows.txt
+++ b/Documentation/gitworkflows.txt
@@ -464,6 +464,221 @@ in patches to figure out the merge base.  See 
linkgit:git-am[1] for
 other options.
 
 
+TRIANGULAR WORKFLOW
+---
+
+Introduction
+
+
+In some projects, contributors cannot push directly to the project but
+have to suggest their commits to the maintainer (e.g. pull requests).
+For these projects, it's common to use what's called a *triangular
+workflow*:
+
+- The project maintainer publishes a repository, called **UPSTREAM** in
+this document, which is a read-only for contributors. They can clone and
+fetch from this repository.
+- Contributors publish their modifications by pushing to a repository,
+called **PUBLISH** in this document, and request a merge.
+- Opening a pull request
+- If the maintainer accepts the changes, he merges them into the
+  **UPSTREAM** repository.
+
+This workflow is commonly used on different platforms like BitBucket,
+GitHub or GitLab which provide a dedicated mechanism for requesting merges.
+
+
+--   -
+| UPSTREAM   |  maintainer   | PUBLISH   |
+|  git/git   |- - - - - - - -|  me/remote|
+--  <-   -
+  \ /
+   \   /
+fetch | \ / ^ push
+  v  \   /  |
+  \ /
+   -
+   |   LOCAL   |
+   -
+
+
+Motivations
+~~~
+
+* Allows contributors to work with Git even if they don't have
+write access to **UPSTREAM**.
+
+Indeed, in a centralized workflow, a contributor without write access
+could write some code but could not send it by itself.  The contributor
+was forced to create a mail which shows the difference between the
+new and the old code, and then send it to a maintainer to commit
+and push it.  This isn't convenient at all, neither for the
+contributor, neither for the maintainer. With the triangular
+workflow, the contributors have the write access on **PUBLISH**
+so they don't have to pass upon maintainer(s).  And only the
+maintainer(s) can push from **PUBLISH** to **UPSTREAM**.
+This is called a distributed workflow (See "DISTRIBUTED WORKFLOWS"
+above).
+
+* Code review is made before integration.
+
+The goal of the triangular workflow is also that the rest of the
+community or the company can review the code before it's in production.
+Everyone can read on **PUBLISH** so everyone can review code
+before the maintainer(s) merge it to **UPSTREAM**.  It also means
+that, in a free software, anyone can propose code without danger
+for the stability of the software.
+
+* Encourages clean history by using `rebase -i` and `push --force` to
+the public fork before the code is merged.
+
+This is just a side-effect of the "review before merge" mentionned
+above but this is still a good point.
+
+
+
+Here are the configuration variables you will need to arrange your
+workflow.
+
+Preparation as a contributor
+
+
+Cloning from **PUBLISH**, which is a fork of **UPSTREAM** or an empty
+repository.
+
+==
+`git clone `
+==
+
+Setting the behavior of push for the triangular workflow:
+
+===
+`git config push.default current`
+===
+
+Adding **UPSTREAM** remote:
+
+===
+`git remote add upstream `
+===
+
+With the `remote add` above, using `git pull upstream` pulls there,
+instead of saying its URL. In addition, the `git pull` command
+(without argument) can be used to pull from **UPSTREAM**.
+
+For each branch requiring a triangular workflow, set
+`branch..remote` and `branch..pushRemote` to set up
+the **UPSTREAM** and **PUBLISH** repositories.
+
+Example with master as :
+===
+* `git config branch.master.remote upstream`
+* `git config branch.master.pushRemote origin`
+===
+
+Staying up-to-date
+~~
+
+Retrieve updates from **UPSTREAM** with `git pull` and send them to
+**PUBLISH** with `git push`.
+
+Making your work available
+~~
+
+The `git push` command sends commits to the **PUBLISH** repository and not to
+the **UPSTREAM** thanks to the configuration you did earlier with the
+`git config remote.pushdefault origin` command.
+
+When a contributor pushes something, the `git config push.default

Re: [PATCH v4 4/6] list-objects: filter objects in traverse_commit_list

[Jeff Hostetler]

On 11/16/2017 9:14 PM, Junio C Hamano wrote:

Jeff King  writes:


Those encodings don't necessarily need to be the same, because they're
about transport. Inside each process we'd have the raw bytes, and encode
them as appropriate to whatever sub-program we're going to pass to (or
not at all if we skip the shell for sub-processes, which is usually a
good idea).


Yes, I share the same feeling.  It does not help that the series
defines its own notion of arg_needs_armor() and uses it to set a
field called requires_armor that is not yet used, the definition of
"armor"ing being each byte getting encoded as two hexadecimal digits
without any sign (which makes me wonder what a receiver of
"deadbeef" would do---did it receive an armored string or a plain
one???).  I do not understand why these strings are not passed as
opaque sequences of bytes and instead converted at this low a layer.


I'm probably being too paranoid.  My fear is that a client could pass
an expression to clone/fetch/fetch-pack that would be sent to the
server and evaluated by the interface between upload-pack and pack-objects.
I'm not worried about the pack-protocol transport.  I'm mainly concerned
in how upload-pack passes that *client-expression* to pack-objects and are
there ways for that to go south on the server with a carefully crafted
expression.

Even if we assume that upload-pack on the server directly invokes
pack-objects (rather than a shell), there still might be issues.
For platforms like Linux which have a native execve() and can pass
args in an argv (and which the sub-process also receives in an argv
in their main()), my paranoia is probably overkill.

But on Windows, where the native interface takes a command-line string
rather than an argv, I was concerned.  Yes, there is code in compat/mingw.c
to quote args when building a command line from an argv (and I'm *not*
saying there are bugs in that), but again maybe I am being paranoid.

I'll take another look and the existing quoting mechanisms and re-eval.

Thanks,
Jeff

Re: [PATCH] Makefile: check that tcl/tk is installed

[Christian Couder]

On Thu, Nov 16, 2017 at 2:35 AM, Junio C Hamano  wrote:
> Christian Couder  writes:
>
>> To improve the current behavior when Tcl/Tk is not installed,
>> let's just check that TCLTK_PATH points to something and error
>> out right away if this is not the case.
>>
>> This should benefit people who actually want to install and use
>> git-gui or gitk as they will have to install Tcl/Tk anyway, and
>> it is better for them if they are told about installing it soon
>> in the build process, rather than if they have to debug it after
>> installing.
>
> Not objecting, but thinking aloud if this change makes sense.
>
> If you are building Git for your own use on the same box, which is
> presumably the majority of "build failed and I have no clue how to
> fix" case that needs help, if you want gui tools, you need to have
> tcl/tk installed anyway, whether you have msgfmt installed.  This
> seems to be the _only_ class of users this patch wants to cater to.
>
> I wonder if we are hurting people who are not in that category.
>
>  - To run gui tools, tcl/tk must be available at runtime, but tcl/tk
>is not necessary in the packager's environment to produce a
>package of Git that contains working git-gui and gitk that will
>be used on an end-user box with tcl/tk installed, as long as the
>packager's environment has a working msgfmt.
>
>  - To process .po files for the gui tools in the packager's
>environment without msgfmt, tcl/tk is required.
>
> I suspect that this change will hurt those who package Git for other
> people.

Maybe a little bit, but in my opinion it should not be a big problem
for them to install Tcl/Tk and its dependencies on the build machine.

> It used to be that, as long as they have msgfmt installed, they only
> needed to _know_ what the path on the users' box to "wish" is, and
> set it to TCLTK_PATH, and if they are distro packagers, most likely
> they already have such an automated set-up working.  Now with this
> change, they are forced to install tcl/tk on their possibly headless
> box where tcl/tk is useless, and worse yet, an attempt to install it
> may bring in tons of unwanted stuff related to X that is irrelevant
> on such a headless development environment.

Yeah, but if they build gitk and git-gui, there is a significant
chance that they build other graphical software too, and that this
will require installing stuff related to X anyway.

> I doubt that this is quite a good trade-off; it feels that this
> burdens packagers a bit too much, and we may need a way to override
> this new check further.

I am ok to let packagers override this new check. For example they
could set a flag like BYPASS_TCLTK_CHECK and the new check would be:

ifndef NO_TCLTK
  ifndef BYPASS_TCLTK_CHECK
 has_tcltk := $(shell type $(TCLTK_PATH) 2>/dev/null)
 ifndef has_tcltk
$(error "Tcl/Tk is not installed ('$(TCLTK_PATH)' not found). Consider
setting NO_TCLTK or installing Tcl/Tk")
 endif
  endif
endif

Of course BYPASS_TCLTK_CHECK would have to be documented at the same
place where NO_TCLTK and TCLTK_PATH are already documented.

In general I think packagers are much more able to deal with those
kinds of problems than most regular developers who want to hack on
Git.
So asking packagers to either set NO_TCLTK or BYPASS_TCLTK_CHECK or to
install Tcl/Tk would not burden them much, especially compared to what
regular developers have to deal with these days when trying to build
Git.

> I think "If I cannot run either wish or
> msgfmt, then barf and give an error message" might at least be
> needed.  Am I misinterpreting the motivation of the patch?

I'd rather add a separate check for msgfmt than mixing the 2 issues,
because I think that unless it has been explicitly told to do so, Git
should not try to build git-gui and gitk in the first place if there
is a big chance that those tools will not work.

[PATCH v2] launch_editor(): indicate that Git waits for user input

[lars . schneider]

From: Junio C Hamano 

When a graphical GIT_EDITOR is spawned by a Git command that opens
and waits for user input (e.g. "git rebase -i"), then the editor window
might be obscured by other windows. The user may be left staring at the
original Git terminal window without even realizing that s/he needs to
interact with another window before Git can proceed. To this user Git
appears hanging.

Show a message in the original terminal and get rid of it when the
editor returns.

Signed-off-by: Junio C Hamano 
Signed-off-by: Lars Schneider 
---

Junio posted the original version of this patch [1] as response to my RFC [2].
I took Junio's patch and slightly changed the commit message as well as the
message printed to the user after GIT_EDITOR is invoked [3].

Thanks,
Lars


[1] https://public-inbox.org/git/xmqqr2syvjxb@gitster.mtv.corp.google.com/
[2] https://public-inbox.org/git/274b4850-2eb7-4bfa-a42c-25a573254...@gmail.com/
[3] https://public-inbox.org/git/daec36c7-ae09-4c9b-acc4-07f2c5f2b...@gmail.com/

Notes:
Base Ref: master
Web-Diff: https://github.com/larsxschneider/git/commit/6fd6d8e682
Checkout: git fetch https://github.com/larsxschneider/git editor-v2 && git 
checkout 6fd6d8e682


 editor.c | 29 +
 1 file changed, 29 insertions(+)

diff --git a/editor.c b/editor.c
index 7519edecdc..23db92d8c6 100644
--- a/editor.c
+++ b/editor.c
@@ -40,6 +40,32 @@ int launch_editor(const char *path, struct strbuf *buffer, 
const char *const *en
const char *args[] = { editor, real_path(path), NULL };
struct child_process p = CHILD_PROCESS_INIT;
int ret, sig;
+   static const char *close_notice = NULL;
+
+   if (isatty(2) && !close_notice) {
+   char *term = getenv("TERM");
+
+   if (term && strcmp(term, "dumb"))
+   /*
+* go back to the beginning and erase the
+* entire line if the terminal is capable
+* to do so, to avoid wasting the vertical
+* space.
+*/
+   close_notice = "\r\033[K";
+   else
+   /* otherwise, complete and waste the line */
+   close_notice = "done.\n";
+   }
+
+   if (close_notice) {
+   fprintf(
+   stderr,
+   "Launched your editor ('%s'). Adjust, save, and 
close the "
+   "file to continue. Waiting for your input... ", 
editor
+   );
+   fflush(stderr);
+   }

p.argv = args;
p.env = env;
@@ -53,11 +79,14 @@ int launch_editor(const char *path, struct strbuf *buffer, 
const char *const *en
sig = ret - 128;
sigchain_pop(SIGINT);
sigchain_pop(SIGQUIT);
+
if (sig == SIGINT || sig == SIGQUIT)
raise(sig);
if (ret)
return error("There was a problem with the editor 
'%s'.",
editor);
+   if (close_notice)
+   fputs(close_notice, stderr);
}

if (!buffer)

base-commit: 89ea799ffcc5c8a0547d3c9075eb979256ee95b8
--
2.15.0

Re: git send-email does not work with Google anymore?!

[Lars Schneider]

> On 23 Oct 2017, at 18:27, Dennis Kaarsemaker  wrote:
> 
> On Thu, 2017-10-05 at 12:52 +0200, Lars Schneider wrote:
>> Hi,
>> 
>> I used to use the Google SMTP server to send my patches to the list with 
>> the following config:
> 
>> 8 ---
> 
>> Apparently that stopped working today. I get this error:
>> 
>>(mbox) Adding cc: Lars Schneider  from line 
>> 'From: Lars Schneider '
>>Password for 'smtp://larsxschnei...@gmail.com@smtp.gmail.com:587':
>>5.7.14 >5.7.14 ...> Please log in via your web browser and
>>5.7.14 then try again.
>>5.7.14  Learn more at
>>5.7.14  https://support.google.com/mail/answer/78754 ... - gsmtp
>> 
>> Of couse I tried to log in via web browser etc. Does anyone else use 
>> Google as SMTP server? If yes, does it work for you?
> 
> For 2fa-protected accounts, this seems to break quite often. I ended up
> setting up a mail relay on my vps for this. If you want, you can use
> git.seveas.net to send patches to the git mailing lists (doesn't work
> for other recipients, I'm not quite stupid enough to run an open mail
> relay...)

:-) ... unfortunately that also means one can not CC people using your
server. Looks like I need to setup my own solution after all :-(

Cheers,
Lars

[PATCH v3 2/8] 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 v1:
 - prefix cleanup_mode enum and constants with commit_msg_

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

diff --git a/builtin/commit.c b/builtin/commit.c
index 
8a877014145435516930c787dec37b8c4ac3da90..d958c2eb2adc9a29dab29340ce9b56daea41fecd
 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;
-
-   if (!template_file || strbuf_read_file(, template_file, 0) <= 0)
-   return 0;
-
-   strbuf_stripspace(, cleanup_mode == 

[PATCH v3 6/8] 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 
a2cf6f5e06ffec5108f0faf43d1a4cb605264c3f..7400df5522037583108534755af6f542117667c2
 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -477,9 +477,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);
 
@@ -657,8 +654,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))
@@ -1347,6 +1342,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.0

[PATCH v3 3/8] 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 
d958c2eb2adc9a29dab29340ce9b56daea41fecd..eb144556bf37b7bf357bd976b94305171b4fd159
 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 
36e03d041f32bcc0fdd1fddebb33b23c7e4d8a70..ef262980c5255d90ee023c0b29c6c1c628b3c7d2
 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"
@@ -752,6 +752,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 
82e57713a2940c5d65ccac013c3f42c55cc12baf..81a2098e900f0aca30e45ed7f19ae4bf3ce682f0
 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -69,4 +69,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 v3 4/8] 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 
eb144556bf37b7bf357bd976b94305171b4fd159..d251cfcebad3476c365492d83803e7821fdfdf2b
 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 
ef262980c5255d90ee023c0b29c6c1c628b3c7d2..6bc8346d42bb3cb1d2dc6a2238dd1b38e4308914
 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"
 
@@ -789,6 +791,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 v3 0/8] sequencer: don't fork git commit

[Phillip Wood]

From: Phillip Wood 

I've updated these based on the feedback for v2. I've dropped the
patch that stopped print_commit_summary() from dying as I think it is
better to die than return an error (see the commit message of the
patch that adds print_commit_summary() for the reasoning). Apart from
that they're minor changes - style fixes and a reworded a commit message.

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 (8):
  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'

 builtin/commit.c  | 289 +++--
 builtin/rebase--helper.c  |  13 +-
 builtin/revert.c  |  15 +-
 sequencer.c   | 486 +-
 sequencer.h   |  23 ++
 t/t3404-rebase-interactive.sh |   4 +
 6 files changed, 561 insertions(+), 269 deletions(-)

-- 
2.15.0

[PATCH v3 7/8] sequencer: load commit related config

[Phillip Wood]

From: Phillip Wood 

Load default values for message cleanup and gpg signing of commits in
preparation for committing without forking 'git commit'.

Signed-off-by: Phillip Wood 
---

Notes:
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 | 13 -
 builtin/revert.c | 15 +--
 sequencer.c  | 34 ++
 sequencer.h  |  1 +
 4 files changed, 60 insertions(+), 3 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 
f8519363a393862b6857acab037e74367c7f2134..68194d3aed950f327a8bc624fa1991478dfea01e
 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -9,6 +9,17 @@ static const char * const builtin_rebase_helper_usage[] = {
NULL
 };
 
+static int git_rebase_helper_config(const char *k, const char *v, void *cb)
+{
+   int status;
+
+   status = git_sequencer_config(k, v, NULL);
+   if (status)
+   return status;
+
+   return git_default_config(k, v, NULL);
+}
+
 int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 {
struct replay_opts opts = REPLAY_OPTS_INIT;
@@ -39,7 +50,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
OPT_END()
};
 
-   git_config(git_default_config, NULL);
+   git_config(git_rebase_helper_config, NULL);
 
opts.action = REPLAY_INTERACTIVE_REBASE;
opts.allow_ff = 1;
diff --git a/builtin/revert.c b/builtin/revert.c
index 
b9d927eb09c9ed87c84681df1396f4e6d9b13c97..1938825efa6ad20ede5aba57f097863aeb33d1d5
 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -31,6 +31,17 @@ static const char * const cherry_pick_usage[] = {
NULL
 };
 
+static int common_config(const char *k, const char *v, void *cb)
+{
+   int status;
+
+   status = git_sequencer_config(k, v, NULL);
+   if (status)
+   return status;
+
+   return git_default_config(k, v, NULL);
+}
+
 static const char *action_name(const struct replay_opts *opts)
 {
return opts->action == REPLAY_REVERT ? "revert" : "cherry-pick";
@@ -208,7 +219,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);
+   git_config(common_config, NULL);
res = run_sequencer(argc, argv, );
if (res < 0)
die(_("revert failed"));
@@ -221,7 +232,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);
+   git_config(common_config, NULL);
res = run_sequencer(argc, argv, );
if (res < 0)
die(_("cherry-pick failed"));
diff --git a/sequencer.c b/sequencer.c
index 
7400df5522037583108534755af6f542117667c2..2c487f9feb83806e5db54a8675c9eb55cc62a5ec
 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -688,6 +688,40 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
return run_command();
 }
 
+static enum commit_msg_cleanup_mode default_msg_cleanup =
+   COMMIT_MSG_CLEANUP_NONE;
+static char *default_gpg_sign;
+
+int git_sequencer_config(const char *k, const char *v, void *cb)
+{
+   if (!strcmp(k, "commit.cleanup")) {
+   int status;
+   const char *s;
+
+   status = git_config_string(, k, v);
+   if (status)
+   return status;
+
+   if (!strcmp(s, "verbatim"))
+   default_msg_cleanup = COMMIT_MSG_CLEANUP_NONE;
+   else if (!strcmp(s, "whitespace"))
+   default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE;
+   else if (!strcmp(s, "strip"))
+   default_msg_cleanup = COMMIT_MSG_CLEANUP_ALL;
+   else if (!strcmp(s, "scissors"))
+   default_msg_cleanup = COMMIT_MSG_CLEANUP_NONE;
+
+   return status;
+   }
+
+   if (!strcmp(k, "commit.gpgsign")) {
+   default_gpg_sign = git_config_bool(k, v) ? "" : NULL;
+   return 0;
+   }
+
+   return git_gpg_config(k, v, NULL);
+}
+
 static int rest_is_empty(const struct strbuf *sb, int start)
 {
int i, eol;
diff --git a/sequencer.h b/sequencer.h
index 
4f616c61a3f3869daf9f427b978c308d6094a978..77cb174b2aaf3972ebb9e6ec379252be96dedd3d
 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -57,6 +57,7 @@ extern const char sign_off_header[];
 
 void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag);
 void append_conflicts_hint(struct strbuf *msgbuf);
+int 

[PATCH v3 8/8] 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 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 | 178 +++-
 1 file changed, 176 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 
2c487f9feb83806e5db54a8675c9eb55cc62a5ec..8362d8fe9f19c61e6339b7a325771344c7a136a8
 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -592,6 +592,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"
@@ -984,6 +996,160 @@ 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;
+   const char *gpg_sign;
+   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;
+   strbuf_addstr(msg, orig_message);
+   }
+   author = amend_author = get_author(message);
+   unuse_commit_buffer(current_head, message);
+   if (!author) {
+   res = 

[PATCH v3 5/8] 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:

 - 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 
d251cfcebad3476c365492d83803e7821fdfdf2b..2043479d37873671d43124dc0cb509d6d9247baa
 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");
-
-   format_commit_message(commit, "%an <%ae>", _ident, );
-   

[PATCH v3 1/8] 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.0

is there a rationale for some sample hooks but not others?

[Robert P. J. Day]

  given that a newly-initialized repo contains samples for some hooks
but not others, is there a simple rationale for why those particular
sample hooks are provided, and not the rest? just curious.

rday

-- 


Robert P. J. Day Ottawa, Ontario, CANADA
http://crashcourse.ca

Twitter:   http://twitter.com/rpjday
LinkedIn:   http://ca.linkedin.com/in/rpjday