Re: [RFC] cover-at-tip

[Nicolas Morey-Chaisemartin]

Le 10/11/2017 à 19:22, Junio C Hamano a écrit :
> Nicolas Morey-Chaisemartin  writes:
>
>> I would need to add "some" level of parsing to am.c to make sure
>> the patch content is just garbage and that there are no actual
>> hunks for that.
>>
>> I did not find any public API that would allow me to do that,
>> although apply_path/parse_chunk would fit the bill.  Is that the
>> right way to approach this ?
> I do not think you would want this non-patch cruft seen at the apply
> layer at all.  Reading a mailbox, with the help of mailsplit and
> mailinfo, and being the driver to create a series of commits is what
> "am" is about, and it would have to notice that the non-patch cruft
> at the beginning is not a patch at all and defer creation of an
> empty commit with that cover material at the end.  For each of the
> other messages in the series that has patches, it will need to call
> apply to update the index and the working tree so that it can make a
> commit, but there is NO reason whatsoever to ask help from apply, whose
> sole purpose is to read a patch and make modifications to the index
> and the working tree, to handle the cover material.
>
>

I agree this is a "am" job. Was just wondering if reusing some of the code from 
apply (and move it so it makes more sense) wouldnd't make more sense than 
rewriting a patch detection function.

Nicolas

Re: [PATCH] Fix NO_LIBPCRE1_JIT to fully disable JIT

[Charles Bailey]

On Mon, Nov 13, 2017 at 12:53:15PM +0900, Junio C Hamano wrote:
> 
> Thanks.  This patch needs a sign-off, by the way.

Signed-off-by: cbaile...@bloomberg.net

(I can resend the full patch if required or if anyone requests futher
changes.

> > But that we should take it anyway regardless of that since it'll *also*
> > work on Linux with your patch, and this logic makes some sense whereas
> > the other one clearly didn't and just worked by pure accident of some
> > toolchain semantics that I haven't figured out yet.
> 
> That is curious and would be nice to know the answer to.

The error that I was getting - if I remember the details of the very
brief debugging session that I performed - was an unaligned memory
access causing a SIGBUS in PCRE code whose function name contained 'jit'
and which was being called indirectly from pcre_study.

My guess is that we are just exposing a pre-existing bug in our Solaris
build of libpcre. Unaligned memory accesses on x86 / x86_64 "only" cause
performance issues rather than fatal signals so even if the same bug
exists on Linux it probably has no noticeable effect (or at least no
noticed effect).

Charles.

Re: [PATCH 4/4] sequencer: Show rename progress during cherry picks

[Junio C Hamano]

Elijah Newren  writes:

> Subject: Re: [PATCH 4/4] sequencer: Show rename progress during cherry picks

Style: s/Show/show/

> When trying to cherry-pick a change that has lots of renames, it is
> somewhat unsettling to wait a really long time without any feedback.
>
> Signed-off-by: Elijah Newren 
> ---
>  sequencer.c | 1 +
>  1 file changed, 1 insertion(+)

Makes sense.


> diff --git a/sequencer.c b/sequencer.c
> index 2b4cecb617..247d93f363 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -448,6 +448,7 @@ static int do_recursive_merge(struct commit *base, struct 
> commit *next,
>   o.branch2 = next ? next_label : "(empty tree)";
>   if (is_rebase_i(opts))
>   o.buffer_output = 2;
> + o.show_rename_progress = 1;
>  
>   head_tree = parse_tree_indirect(head);
>   next_tree = next ? next->tree : empty_tree();

Re: [PATCH 3/4] progress: Fix progress meters when dealing with lots of work

[Junio C Hamano]

Elijah Newren  writes:

> Subject: Re: [PATCH 3/4] progress: Fix progress meters when dealing with lots 
> of work

Style: s/Fix/fix/;

> The possibility of setting merge.renameLimit beyond 2^16 raises the
> possibility that the values passed to progress can exceed 2^32.  For my
> usecase of interest, I only needed to pass a value a little over 2^31.  If
> I were only interested in fixing my usecase, I could have simply changed
> last_value from int to unsigned, and casted each of rename_dst_nr and
> rename_src_nr (in merge-recursive.c) from int to unsigned just before
> multiplying them.  However, as long as we're making changes to allow
> larger progress meters, we may as well make a little more room in general.
> Use uint64_t, because it "ought to be enough for anybody".  :-)

The middle part of the log message may waste more mental bandwidth
of readers than it is worth.  It might have gave you satisfaction to
be able to vent, but don't (the place to do so is after the three
dash lines).

I am not sure if we want all codepaths to do 64-bit math for
progress meter, but let's see what others would think.

> -static int display(struct progress *progress, unsigned n, const char *done)
> +static int display(struct progress *progress, uint64_t n, const char *done)
>  {
>   const char *eol, *tp;
>  
> @@ -106,7 +106,7 @@ static int display(struct progress *progress, unsigned n, 
> const char *done)
>   if (percent != progress->last_percent || progress_update) {
>   progress->last_percent = percent;
>   if (is_foreground_fd(fileno(stderr)) || done) {
> - fprintf(stderr, "%s: %3u%% (%u/%u)%s%s",
> + fprintf(stderr, "%s: %3u%% (%lu/%lu)%s%s",

Are these (and there are probably other instances in this patch) %lu
correct?

Re: [PATCH 1/4] sequencer: Warn when internal merge may be suboptimal due to renameLimit

[Junio C Hamano]

Elijah Newren  writes:

> Subject: Re: [PATCH 1/4] sequencer: Warn when internal merge may be 
> suboptimal due to renameLimit

Style: please downcase s/Warn/warn/ to fit this better in "shortlog --no-merges"
output.

> Having renamed files silently treated as deleted/modified conflicts
> instead of correctly resolving the renamed/modified case has caused lots
> of pain for some users.  Eventually, some of them figured out to set
> merge.renameLimit to something higher, but without feedback about what
> value it should have, they were just repeatedly guessing and retrying.

It would have been _much_ easier to read if you refrained from
stating the gripes more prominently than the source of the gripe
that you are fixing.  E.g.

If one side of a merge have renamed more paths than
merge.renamelimit since the sides diverged, the recursive
merge strategy stops detecting renames and instead leaves
these many paths as delete/modify conflicts, without warning
what is going on and giving hints on how to tell Git that it
is allowed to spend more cycles to detect renames.

perhaps.

The addition of a call to diff_warn_rename_limit looks quite
sensible.

Thanks.

> Signed-off-by: Elijah Newren 
> ---
>  sequencer.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/sequencer.c b/sequencer.c
> index f2a10cc4f2..2b4cecb617 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -462,6 +462,7 @@ static int do_recursive_merge(struct commit *base, struct 
> commit *next,
>   if (is_rebase_i(opts) && clean <= 0)
>   fputs(o.obuf.buf, stdout);
>   strbuf_release();
> + diff_warn_rename_limit("merge.renamelimit", o.needed_rename_limit, 0);
>   if (clean < 0)
>   return clean;

Re: [PATCH v3 0/4] Hash abstraction

[Junio C Hamano]

"brian m. carlson"  writes:

> This is a series proposing a basic abstraction for hash functions.
>
> The full description of the series can be found here:
> https://public-inbox.org/git/20171028181239.59458-1-sand...@crustytoothpaste.net/
>
> At the bottom of this message is the output of git tbdiff between v2 and
> v3.
>
> Changes from v2:
> * Remove inline.
> * Add dummy functions that call die for unknown hash implementation.
> * Move structure definitions to hash.h and include git-compat-util.h in
>   hash.h.
> * Rename current_hash to the_hash_algo.
> * Use repo_set_hash_algo everywhere we set the hash algorithm for a
>   struct repository.

Change for all of the above in this series looked sensible to me.
Thank, will queue.

Re: [PATCH] doc: Remove explanation of "--" from several man pages

[Junio C Hamano]

"Robert P. J. Day"  writes:

> There is no value in individual man pages explaining the purpose of
> the "--" separator.
>
> Signed-off-by: Robert P. J. Day 
>
> ---
>
>   unless every man page explains that option, it's pointless to have
> just *some* man pages explaining it, so might as well remove it from
> all of them.

Please do not remove diffstat that format-patch gave you at this
point.  While commenting on the hunk on "git add", I wanted to see
if you touched "git rm", and the diffstat at front _is_ the go-to
place to do so for reviewers.

> diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
> index b700beaff..69d625285 100644
> --- a/Documentation/git-add.txt
> +++ b/Documentation/git-add.txt
> @@ -180,11 +180,6 @@ for "git add --no-all ...", i.e. ignored 
> removed files.
>   bit is only changed in the index, the files on disk are left
>   unchanged.
>
> -\--::
> - This option can be used to separate command-line options from
> - the list of files, (useful when filenames might be mistaken
> - for command-line options).
> -

I do not think if this removal alone is a good idea.  

Before this can happen, the description for "--" in other pages,
(like gitcli(7), may need to be extended.  Right now, gitcli's
mention of "--" is only about turning off disambiguation between
revs and pathspecs, and it does not cover this case

$ >./--foo-bar
$ git add -- --foo-bar

even though the description you are removing would have helped the
reader to understand why "--" is there.  The hunk on "git rm" shares
the same issue.

>  Configuration
>  -
> diff --git a/Documentation/git-check-attr.txt 
> b/Documentation/git-check-attr.txt
> index aa3b2bf2f..0ae2523e0 100644
> --- a/Documentation/git-check-attr.txt
> +++ b/Documentation/git-check-attr.txt
> @@ -36,10 +36,6 @@ OPTIONS
>   If `--stdin` is also given, input paths are separated
>   with a NUL character instead of a linefeed character.
>
> -\--::
> - Interpret all preceding arguments as attributes and all following
> - arguments as path names.
> -

This also has a similar issue.  "--" here is not between revs and
pathspecs but is between attributes and pathspecs.

> diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
> index d153c17e0..93ebb020c 100644
> --- a/Documentation/git-ls-files.txt
> +++ b/Documentation/git-ls-files.txt
> @@ -171,9 +171,6 @@ Both the  in the index ("i/")
>  and in the working tree ("w/") are shown for regular files,
>  followed by the  ("attr/").
>
> -\--::
> - Do not interpret any more arguments as options.

These removals would become a good idea, once we say that we would
use "--" to mean "you will not see any --flags after this point" (as
commonly seen in programs that are not Git) somewhere central like
gitcli(7).

Re: [PATCH] bisect run: die if no command is given

[Junio C Hamano]

Stephan Beyer  writes:

> It was possible to invoke "git bisect run" without any command.
> This considers all commits as good commits since "$@"'s return
> value for empty $@ is 0.
>
> This is most probably not what a user wants (otherwise she would
> invoke "git bisect run true"), so not providing a command now
> results in an error.
>
> Signed-off-by: Stephan Beyer 
> ---

Makes sense to me.  Thanks, will queue.

>  git-bisect.sh | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/git-bisect.sh b/git-bisect.sh
> index 0138a8860..a69e43656 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -450,6 +450,8 @@ bisect_replay () {
>  bisect_run () {
>   bisect_next_check fail
>  
> + test -n "$*" || die "$(gettext "bisect run failed: no command 
> provided.")"
> +
>   while true
>   do
>   command="$@"

Re: [PATCH v16 Part II 5/8] bisect--helper: `bisect_next_check` shell function in C

[Junio C Hamano]

Stephan Beyer  writes:

> Hi again ;)
> ...
> In both of the above "error" calls, you should drop the final "\n"
> because "error" does that already.
>
> On the other hand, you have dropped the "\n"s of the orginal error
> messages. So it should probably be
>
>  _("You need to give me at least one %s and %s revision.\n"
>"You can use \"git bisect %s\" and \"git bisect %s\" for that.")
>
> and
>
>  _("You need to start by \"git bisect start\".\n"
>"You then need to give me at least one %s and %s revision.\n"
>"You can use \"git bisect %s\" and "\"git bisect %s\" for that.")
>
> Stephan

Thanks for reviews (not just this patch, but for reviews on other
patches, too).

Re: [add-default-config 2/5] adding default to color

[Jeff King]

On Mon, Nov 13, 2017 at 12:40:16PM +0900, Junio C Hamano wrote:

> As an aside.  Over time we accumulated quite a many actions that are
> all mutually exclusive by nature.  I have a feeling that we might be
> better off to move away from this implementation.  The only thing
> that we are getting from the current one-bit-in-a-flag-word is that
> we can name the variable "actions" (instead of "action") to pretend
> as if we can be given more than one, and then having to check its
> value with HAS_MULTI_BITS(actions) to confuse ourselves.
> 
> Instead, perhaps we should introduce an "enum action" that includes
> ACTION_UNSPECIFIED that is the initial value for the "action"
> variable, which gets set to ACTION_GET, etc. with OPT_SET_INT().  If
> we really care about erroring out when given
> 
>   $ git config --add --get foo.bar
> 
> instead of the "last one wins" semantics, we can use OPT_CMDMODE.
> 
> The above is of course outside the scope of this series, and I am
> not sure if it should be done as a preparatory or a follow-up
> clean-up.

Yes, I agree that it's a little confusing, and that an enum is a better
representation.  The TYPE constants have the same problem.

I _think_ we could use OPT_CMDMODE() for those, too. Despite the name,
there is nothing in the parse-options error message that would be
inappropriate for something that isn't a cmdmode. Though I care a lot
less about "--bool --int" reporting an error (instead of last-one-wins)
than I do about "--get --set".

-Peff

Re: [PATCH] Fix NO_LIBPCRE1_JIT to fully disable JIT

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> On Sun, Nov 12 2017, Charles Bailey jotted:
>
>> From: Charles Bailey 
>>
>> If you have a pcre1 library which is compiled with JIT enabled then
>> PCRE_STUDY_JIT_COMPILE will be defined whether or not the
>> NO_LIBPCRE1_JIT configuration is set.
>>
>> This means that we enable JIT functionality when calling pcre_study
>> even if NO_LIBPCRE1_JIT has been explicitly set and we just use plain
>> pcre_exec later.
>>
>> Fix this by using own macro (GIT_PCRE_STUDY_JIT_COMPILE) which we set to
>> PCRE_STUDY_JIT_COMPILE only if NO_LIBPCRE1_JIT is not set and define to
>> 0 otherwise, as before.
>> ---
>>
>> I was bisecting an issue with the PCRE support that was causing a test
>> ...
>
> [CC-ing Junio]
>
> Thanks a lot. This patch looks good to me.

Thanks.  This patch needs a sign-off, by the way.

> But that we should take it anyway regardless of that since it'll *also*
> work on Linux with your patch, and this logic makes some sense whereas
> the other one clearly didn't and just worked by pure accident of some
> toolchain semantics that I haven't figured out yet.

That is curious and would be nice to know the answer to.

Re: [PATCH] Make t4201-shortlog.sh test more robust

[Junio C Hamano]

Jeff King  writes:

> You're also really doing two things to fix the problem here, either one
> of which would have been sufficient: increasing the abbreviation size
> and using test_tick to get a deterministic run.
> ...
> Doing it once is enough to make the test deterministic, and for this
> particular case we don't actually care at all whether all of the commits
> have the exact same timestamp. So I think it's fine.

;-)

Re: [add-default-config 2/5] adding default to color

[Junio C Hamano]

Jeff King  writes:

>> @@ -47,6 +48,7 @@ static int show_origin;
>>  #define ACTION_GET_COLOR (1<<13)
>>  #define ACTION_GET_COLORBOOL (1<<14)
>>  #define ACTION_GET_URLMATCH (1<<15)
>> +#define ACTION_GET_COLORORDEFAULT (1<<16)
>
> I'm not sure I understand this part, though. Providing a default should
> be something that goes along with a "get" action, but isn't its own
> action.

I agree that it is not.

As an aside.  Over time we accumulated quite a many actions that are
all mutually exclusive by nature.  I have a feeling that we might be
better off to move away from this implementation.  The only thing
that we are getting from the current one-bit-in-a-flag-word is that
we can name the variable "actions" (instead of "action") to pretend
as if we can be given more than one, and then having to check its
value with HAS_MULTI_BITS(actions) to confuse ourselves.

Instead, perhaps we should introduce an "enum action" that includes
ACTION_UNSPECIFIED that is the initial value for the "action"
variable, which gets set to ACTION_GET, etc. with OPT_SET_INT().  If
we really care about erroring out when given

$ git config --add --get foo.bar

instead of the "last one wins" semantics, we can use OPT_CMDMODE.

The above is of course outside the scope of this series, and I am
not sure if it should be done as a preparatory or a follow-up
clean-up.

Re: [Query] Separate hooks for Git worktrees

[Viresh Kumar]

On 10-11-17, 10:00, Stefan Beller wrote:
> Well it is the same project with different upstream workflows.
> For example I would imagine that Viresh wants to cherry-pick
> from one branch to another, or even send the same patch
> (just with different commit messages, with or without the
> ChangeId) to the different upstreams?

Right.

-- 
viresh

Re: [RFC PATCH v3 2/4] branch: re-order function arguments to group related arguments

[Kaartic Sivaraam]

On Mon, 2017-11-13 at 11:32 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam  writes:
> 
> > I've tried to improve it, does the following paragraph sound clear
> > enough?
> > 
> > branch: group related arguments of create_branch()
> > 
> > New arguments were added to create_branch() whenever the need
> > arised and they were added to tail of the argument list. This
> > resulted in the related arguments not being close to each other.
> 
> OK, I understand what you wanted to say.  But I do not think that is
> based on a true history.
> 
>  - f9a482e6 ("checkout: suppress tracking message with "-q"",
>2012-03-26) adds 'quiet' just after 'clobber_head', exactly
>because they are related, and leaves 'track' at the end.
> 
>  - 39bd6f72 ("Allow checkout -B  to update the
>current branch", 2011-11-26) adds 'clobber_head' not at the end but
>before 'track', which is left at the end.  
> 
>  - c847f537 ("Detached HEAD (experimental)", 2007-01-01) split 'start'
>into 'start_name' and 'start_sha1' (the latter was laster removed)
>and this was not a mindless "add at the end", either.
> 
>  - 0746d19a ("git-branch, git-checkout: autosetup for remote branch
>tracking", 2007-03-08) did add track at the end, but that is
>justifiable, as it has no relation to any other parameter.
> 

Seems I wasn't careful enough in noticing how the arguments were added.
I seemed to have overlooked the fact that 39bd6f72 added 'clobber_head'
"before" track which resulted in the vague commit message. Anyways,
thanks for taking the time to dig into this.


> You could call 39bd6f72 somewhat questionable as 'clobber_head' is
> related to 'force' more strongly than it is to 'reflog' [*1*], but
> it is unfair to blame anything else having done a mindless "add at
> the end".
> 

Yep, you're right. How does the following sound?

branch: group related arguments of create_branch()

39bd6f726 (Allow checkout -B  to update the current
branch, 2011-11-26) added 'clobber_head' (now, 'clobber_head_ok')
"before" 'track' as 'track' was closely related 'clobber_head' for
the purpose the commit wanted to achieve. Looking from the perspective
of how the arguments are used it turns out that 'clobber_head' is
more related to 'force' than it is to 'track'.

So, re-order the arguments to keep the related arguments close
to each other.

-- 
Kaartic

Re: [PATCH v1 2/2] worktree: make add dwim

[Junio C Hamano]

Thomas Gummerer  writes:

> I'm a bit torn about hiding his behind an additional flag in git
> worktree add or not.  I would like to have the feature without the
> additional flag, but it might break some peoples expectations, so
> dunno.

Yeah, I agree with the sentiment.  But what expectation would we be
breaking, I wonder (see below)?

At the conceptual level, it is even more unfortunate that "git
worktree --help" says this for "git worktree add  []":

If `` is omitted and neither `-b` nor `-B` nor
`--detach` used, then, as a convenience, a new branch based at
HEAD is created automatically, as if `-b $(basename )` was
specified.

which means that it already does a DWIM, namely "since you didn't
say what branch to create to track what other branch, we'll fork one
for you from the HEAD, and give a name to it".  That may be a useful
DWIM for some existing users sometimes, and you may even find it
useful some of the time but not always.  Different people mean
different things in different situations, and there is no single
definition for DWIMming that would fit all of them.

Which in turn means that the new variable name DWIM_NEW_BRANCH and
the new option name GUESS are way underspecified.  You'd need to
name them after the "kind" of dwim; otherwise, not only the future
additions for new (third) kind of dwim would become cumbersome, but
readers of the code would be confused if they are about the existing
dwim (fork a new branch from HEAD and give it a name guessed by the
pathname) or your new dwim.

This also needs a documentation update.  Unlike the existing DWIM,
it is totally unclear when you are dwimming in absence of which
option.

I am guessing that, when you do not have a branch "topic" but your
upstream does, saying

$ git worktree add ../a-new-worktree topic

would create "refs/heads/topic" to build on top of
"refs/remotes/origin/topic" just like "git checkout topic" would.

IOW, when fully spelled out:

$ git branch -t -b topic origin/topic
$ git worktree add ../a-new-worktree topic

is what your DWIM does?  Am I following you correctly?

If so, as long as the new DWIM kicks in ONLY when "topic" does not
exist, I suspect that there is no backward compatibility worries.
The command line

$ git worktree add ../a-new-worktree topic

would just have failed because three was no 'topic' branch yet, no?

Re: [PATCH v1 1/2] checkout: factor out functions to new lib file

[Junio C Hamano]

Thomas Gummerer  writes:

> Factor the functions out, so they can be re-used from other places.  In
> particular these functions will be re-used in builtin/worktree.c to make
> git worktree add dwim more.
>
> While there add a description to the function.
>
> Signed-off-by: Thomas Gummerer 
> ---
>
> I'm not sure where these functions should go ideally, they don't seem
> to fit in any existing library file, so I created a new one for now.
> Any suggestions for a better home are welcome!
>
>  Makefile   |  1 +
>  builtin/checkout.c | 41 +
>  checkout.c | 43 +++
>  checkout.h | 14 ++
>  4 files changed, 59 insertions(+), 40 deletions(-)
>  create mode 100644 checkout.c
>  create mode 100644 checkout.h

I am not sure either.  I've always considered that entry.c is the
libified thing codepath that want to do a "checkout" should call,
but the functions that you are moving is at a "higher layer" that
does not concern the core-git data structures (i.e. the 'tracking'
is a mere "user" of the ref API, unlike things in entry.c such as
checkout_entry() that is a more "maintainer" of the index integrity),
so perhaps it makes sense to have new file for it.

> diff --git a/checkout.c b/checkout.c
> new file mode 100644
> index 00..a9cf378453
> --- /dev/null
> +++ b/checkout.c
> @@ -0,0 +1,43 @@
> +#include "cache.h"
> +

A useless blank line.

> +#include "remote.h"
> +

This one is useful ;-)

> +struct tracking_name_data {
> ...
> diff --git a/checkout.h b/checkout.h
> new file mode 100644
> index 00..9272fe1449
> --- /dev/null
> +++ b/checkout.h
> @@ -0,0 +1,14 @@
> +#ifndef CHECKOUT_H
> +#define CHECKOUT_H
> +
> +#include "git-compat-util.h"
> +#include "cache.h"

Try "git grep -e git-compat-util Documentation/CodingGuidelines" and
just keep inclusion of "cache.h".

Re: [RFC PATCH v3 2/4] branch: re-order function arguments to group related arguments

[Junio C Hamano]

Kaartic Sivaraam  writes:

> I've tried to improve it, does the following paragraph sound clear
> enough?
>
> branch: group related arguments of create_branch()
> 
> New arguments were added to create_branch() whenever the need
> arised and they were added to tail of the argument list. This
> resulted in the related arguments not being close to each other.

OK, I understand what you wanted to say.  But I do not think that is
based on a true history.

 - f9a482e6 ("checkout: suppress tracking message with "-q"",
   2012-03-26) adds 'quiet' just after 'clobber_head', exactly
   because they are related, and leaves 'track' at the end.

 - 39bd6f72 ("Allow checkout -B  to update the
   current branch", 2011-11-26) adds 'clobber_head' not at the end but
   before 'track', which is left at the end.  

 - c847f537 ("Detached HEAD (experimental)", 2007-01-01) split 'start'
   into 'start_name' and 'start_sha1' (the latter was laster removed)
   and this was not a mindless "add at the end", either.

 - 0746d19a ("git-branch, git-checkout: autosetup for remote branch
   tracking", 2007-03-08) did add track at the end, but that is
   justifiable, as it has no relation to any other parameter.

You could call 39bd6f72 somewhat questionable as 'clobber_head' is
related to 'force' more strongly than it is to 'reflog' [*1*], but
it is unfair to blame anything else having done a mindless "add at
the end".



[Footnote]

*1* I actually think the commit added 'clobber_head' because for the
purpose of what the commit did, it closely was related to 'track',
turning  into .
Arguably, it could have done  instead.

Re: [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming

[Kaartic Sivaraam]

On Sunday 12 November 2017 11:53 PM, Kevin Daudt wrote:

On Thu, Nov 02, 2017 at 12:24:07PM +0530, Kaartic Sivaraam wrote:

From: Kaartic Sivaraam 

When trying to rename an inexistent branch to with a name of a branch


This sentence does not read well. Probably s/with a/the/ helps.



Thanks. Seems I missed it somehow. Will fix it.


that already exists the rename failed specifying the new branch name
exists rather than specifying that the branch trying to be renamed
doesn't exist.

[..]

Note: Thanks to the strbuf API that made it possible to easily construct
the composite error message strings!


I'm not sure this note adds a lot, since the strbuf API is not that new.



That was a little attribution I wanted make to the strbuf API as this 
was the first time I leveraged it to this extent and I was surprised by 
the way it made string manipulation easier in C. Just documented my 
excitation. In case it seems to be noise (?) which should removed, let 
me know.


---
Kaartic

Re: [PATCH v2] gpg-interface: strip CR chars for Windows gpg2

[Junio C Hamano]

Eric Sunshine  writes:

> Thanks for the re-roll...
>
> On Sun, Nov 12, 2017 at 8:07 AM, Jerzy Kozera  wrote:
>> This fixes the issue with newlines being \r\n and not being displayed
>> correctly when using gpg2 for Windows, as reported at
>> https://github.com/git-for-windows/git/issues/1249
>
> It's still not clear from this description what "not being displayed
> correctly" means. Ideally, the commit message should stand on its own,
> explaining exactly what problem the patch is solving, without the
> reader having to chase URLs to pages (which might disappear). If you
> could summarize the problem and solution in your own words in such a
> way that your description itself conveys enough information for
> someone not familiar with that problem report to understand the
> problem, then that would likely make a good commit message.

Thanks.  I was wondering if I am the only one who does not
understand what the revised wording wants to say.

>> @@ -145,6 +145,20 @@ const char *get_signing_key(void)
>> +/* Strip CR from the CRLF line endings, in case we are on Windows. */
>> +static void strip_cr(struct strbuf *buffer, size_t bottom) {
>
> It's not at all clear what 'bottom' means. In the original, when the
> code was inline, the surrounding context would likely have given a
> good clue to the meaning of 'bottom', but here stand-alone, it conveys
> little or nothing. Perhaps a better name for this argument would be
> 'start_at' or 'from' or something.

I personally do not mind 'bottom' (especially when it appears in
contrast to 'top') too much, but start_at would be much clearer.

>> +   size_t i, j;
>> +   for (i = j = bottom; i < buffer->len; i++)
>> +   if (!(i < buffer->len - 1 &&
>> +   buffer->buf[i] == '\r' &&
>> +   buffer->buf[i + 1] == '\n')) {
>
> Hmm, was this tested? If I'm reading this correctly, this strips out
> the entire CRLF pair, whereas the original code only stripped the CR
> and left what followed it (typically LF) alone. Junio's suggestion was
> to enhance this to be more careful and strip CR only when followed
> immediately by LF (but to leave the LF intact). Therefore, this seems
> like a regression.
>
>> +   if (i != j)
>> +   buffer->buf[j] = buffer->buf[i];
>> +   j++;
>> +   }

I think the "negate the entire thing" condition confuses the
readers.  The negated condition is "Do we still have enough bytes to
see if we are looking at CRLF?  Are we at CR?  Is the one byte
beyond what we have a LF?  Do all of these three conditions hold
true?"  If not, i.e. for all the bytes on the line except for that
narrow "we are at CR of a CRLF sequence" case, the body of the loop
makes a literal copy and advances the destination pointer 'j'.  The
only thing that is skipped is CR that comes at the beginning of a
CRLF sequence.

So I think the loop does what it wants to do.

In any case, I think this should be a two patch series---one with
the code as-is with a better explanation, but without "make sure
only CR in CRLF and no other CR are stripped" improvement, and the
other as a follow-up to it to make the improvement.

Thanks.

Re: should "git bisect" support "git bisect next?"

[Junio C Hamano]

Theodore Ts'o  writes:

> On Sun, Nov 12, 2017 at 03:21:57PM +0100, Christian Couder wrote:
>> 
>> Yeah I agree that it might be something interesting for the user to do.
>> But in this case the sequence in which you give the good and the bad
>> commits is not important.
>> Only the last bad commit and the set of good commits that were given
>> are important.
>
> Is it really true that of the bad commits, only the last one is significant?
>
> Suppose we have a git tree that looks like this:
>
>   *---*---*---*---*---*---M2---*---B1
>   ||
>   G1--*--D1---*---*---*---B2-\ |
>   |   \/
>   *---*---*---B3--*---M1--/
>
> If we know that commits B2 and B3 are bad, if we assume that all
> commits before the "bad" commit are good, all commits after the "bad"
> commit are bad, can we not deduce that commit D1 should also be "bad"?

You are correct.  Christian fell into an understandable and common
confusion.  It is true that we only maintain one significant bad
(i.e. the breakage that is known-ealiest so far), but that oldest
bad is the result of the bisection taking into account all the 'bad'
we have got in sequence so far, not necessarily the same as, and
hopefully way better than, the last bad the user gave from the
command line.  In your topology, "git bisect log" would contain "bad
B1", "bad B2", and "bad B3", and when the earlier session that
produced that log saw these three bad commits, it would have marked
D1 as the known-earliest bad one.

Taking the last-given bad B3 is suboptimal than that.

Re: [PATCH] builtin/describe.c: describe a blob

[Junio C Hamano]

Stefan Beller  writes:

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

Thanks for finishing the topic you started.

> @@ -11,6 +11,7 @@ SYNOPSIS
>  [verse]
>  'git describe' [--all] [--tags] [--contains] [--abbrev=] [...]
>  'git describe' [--all] [--tags] [--contains] [--abbrev=] --dirty[=]
> +'git describe' 

OK.

> diff --git a/builtin/describe.c b/builtin/describe.c
> index 9e9a5ed5d4..acfd853a30 100644
> --- a/builtin/describe.c
> +++ b/builtin/describe.c
> ...
>  static void describe(const char *arg, int last_one)
>  {
> ...
> @@ -445,11 +497,18 @@ static void describe(const char *arg, int last_one)
> ...
> + cmit = lookup_commit_reference_gently(, 1);
> +
> + if (cmit)
> + describe_commit(, );
> + else if (lookup_blob()) {
> + if (all || tags || longformat || first_parent ||
> + patterns.nr || exclude_patterns.nr ||
> + always || dirty || broken)
> + die(_("options not available for describing blobs"));
> + describe_blob(oid, );

I am not sure if I agree with some of them.

> + } else
> + die(_("%s is neither a commit nor blob"), arg);

This side I would agree with, though.

The caller of the describe() function is either

 * 'git describe' makes a single call to it with "HEAD" and
   exits; or
 * 'git describe A B C...' makes one call to it for each of these
   command line arguments.

And 'git describe ' code is most likely trigger from the latter,
as it is not likely for HEAD to be pointing at a blob.

$ blob=$(git rev-parse master:Makefile)
$ git describe --always master $blob

and trigger the above check.  Is the check against "always" useful,
or is it better to simply ignore it while describing $blob, but
still keeping it in effect while describing 'master'?

The 'dirty' and 'broken' check is redundant because we would have
already errored out if either of them is set before calling describe()
on user-supplied object names.

If I understand the way "describe " works correctly, it
traverses the history with objects, doing a moral equivalent of
"rev-list --objects", stops when it finds the blob object with the
given name, and when it stops, it knows the commit object that
contains the blob and path in that commit to the blob.  Then the
commit is described to be a human-readable string, so that the path
can be concatenated after it.

Aren't these options that affect how a commit object is described in
effect and useful when you do the "internal" describe of the commit
you found the blob object in?  IOW, wouldn't this

$ git describe --exclude='*-wip' $blob

make sure that in its output $commit:$path, $commit is not described
in terms of any "wip" refs?

Re: [PATCH v1] fsmonitor: simplify determining the git worktree under Windows

[Junio C Hamano]

Ben Peart  writes:

> I haven't tested the non Windows paths but the patch looks reasonable.

I do not think the above line part of the proposed log message for
this patch ;-)  I guess I'll strip these earlier parts and leave
only the last paragraph while queuing.

>
> This inspired me to get someone more familiar with perl (thanks Johannes)
> to revisit this code for the Windows side as well.  The logic for
> determining the git worktree when running on Windows is more complex
> than necessary.  It also spawns multiple processes (uname and cygpath)
> which slows things down.
>
> Simplify and speed up the process of finding the git worktree when
> running on Windows by keeping it in perl and avoiding spawning helper
> processes.
>
> Signed-off-by: Ben Peart 
> Signed-off-by: Johannes Schindelin 
> ---

The patch looks reasonable ;-)  Thanks.

> +if ($^O =~ 'msys' || $^O =~ 'cygwin') {
> + $git_work_tree = Win32::GetCwd();
> + $git_work_tree =~ tr/\\/\//;
>  } else {
>   require Cwd;
>   $git_work_tree = Cwd::cwd();

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

[Gennady Kupava]

Hi Jeff, thanks for such detailed review and additional testing.

Below are just some discussion related review comments,
Please expect patch itself on some evening during coming week.

On 12 November 2017 at 14:17, Jeff King  wrote:
>
> On Sat, Nov 11, 2017 at 07:28:58PM +, gennady.kup...@gmail.com wrote:
>
> > From: Gennady Kupava 
> >
> > Signed-off-by: Gennady Kupava 
>
> Thanks, and welcome to the list.
>
>
> > ---
> > One of the tasks siggested in scope of 'Git sprint weekend'.
> > Couple of chioces made here:
>
> These kinds of choices/caveats (along with the motivation for the patch)
> should go into the commit message so people running "git log" later can
> see them.

Will split into two patches.

>
> >  1. Use constant instead of NULL to indicate default trace level, this just
> > makes everything simpler.
>
> I think this makes sense, as we were translating NULL into this default
> struct behind the scenes already. As we discussed off-list, this does
> mean that you can no longer do:
>
>   trace_printf_key(NULL, "foo");
>
> to send to the default key, and instead must do:
>
>   trace_printf("foo");
>
> The second one has always been the preferred way of spelling it, but the
> first one happened to work. So the only fallout would be if somebody is
> using the non-preferred method, they'd now segfault. There aren't any
> such calls in the current code base, though, and I find it rather
> unlikely that there would be new instances in topics other people are
> working on.
>
> I think it may be worth splitting that out into a separate preparatory
> patch to make it more clear what's going on (especially if our
> assumption turns out wrong and somebody does end up tracing a problem
> back to it).


Logic which lead me to removing NULL:
_If_ we want to do some kind of trivial check before calling functions
from other
translation units (which is always real function call), we have to
have some kind
of status immediately available in .h file.
I saw two options here:
 - move whole 'normalization' procedure to .h
 - remove 'normalization' procedure

Reasons removal option was chosen:
 - I found no code which actually uses NULL as a parameter option.
 - Given the nature of the key parameter ( struct key* )
I was just really unable to find any reasonable use
case there caller really want to pass NULL, so it seemed to me that
best solution
is to remove it. I still can imagine something like dynamic trace
category coming from
some kind of configuration file. In such special case there caller
might need it,
he might just do check in calling code once or just simply add it.
 - Squash big more performance as we do not need to do this
normalization on every call.

I will elaborate on 'moving whole implementation later'

>
> >  2. Move just enough from implementation to header, to be able to do check
> > before calling actual functions.
>
> Makes sense.
>
> >  3. Most controversail: do not support optimization for older
> > compilers not supporting variadic templates in macroses. Problem
> > is that theoretically someone may write some complicated trace
> > while would work in 'modern' compiler and be too slow in more
> > 'classic' compilers, however expectation is that risk of that is
> > quite low and 'classic' compilers will go away eventually.
>
> I think this is probably acceptable. I know we've discussed elsewhere
> recently how common such compilers actually are, and whether we could
> give up on them altogether.
>
> If we wanted to, we could support that case by using inline functions in
> the header (though it does make me wonder if compilers that old also do
> not actually end up inlining).
>
> 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).
>

Good.

>
> > diff --git a/trace.c b/trace.c
> > index 7508aea02..180ee3b00 100644
> > --- a/trace.c
> > +++ b/trace.c
> > @@ -25,26 +25,14 @@
> >  #include "cache.h"
> >  #include "quote.h"
> >
> > -/*
> > - * "Normalize" a key argument by converting NULL to our trace_default,
> > - * and otherwise passing through the value. All caller-facing functions
> > - * should normalize their inputs in this way, though most get it
> > - * for free by calling get_trace_fd() (directly or indirectly).
> > - */
> > -static void normalize_trace_key(struct trace_key **key)
> > -{
> > - static struct trace_key trace_default = { "GIT_TRACE" };
> > - if (!*key)
> > - *key = _default;
> > -}
> > +struct trace_key trace_default_key = { TRACE_KEY_PREFIX, 0, 0, 0 };
>
> Makes sense. We no longer auto-normalize, but just expect the
> appropriate functions to pass a pointer to the default key themselves.

This is again (more details below) choice of how much implementation details we
want to move into header file.

>
>
> > +struct trace_key trace_perf_key 

[PATCH v2 0/2] Convert SubmittingPatches to AsciiDoc

[brian m. carlson]

This series converts SubmittingPatches into AsciiDoc and builds it as
part of the technical documentation.  The goal is to provide a format
that is more easily linkable both from our website and by others, as the
Git Project's patch submission standards are widely referenced and used
as examples.

Changes from v1:
* Switch all branch names, paths, and commit message trailers to use
  backticks.
* Move footnotes directly above relevant paragraph.
* Fix quoting of mail message.

brian m. carlson (2):
  Documentation: enable compat-mode for Asciidoctor
  Documentation: convert SubmittingPatches to AsciiDoc

 Documentation/.gitignore|   1 +
 Documentation/Makefile  |   6 +
 Documentation/SubmittingPatches | 340 +---
 3 files changed, 189 insertions(+), 158 deletions(-)

[PATCH v2 2/2] Documentation: convert SubmittingPatches to AsciiDoc

[brian m. carlson]

The SubmittingPatches document is often cited by outside parties as an
example of good practices to follow, including logical, independent
commits; patch sign-offs; and sending patches to a mailing list.
Currently, people who want to cite a particular section tend to either
refer to it by name and let the interested party search through the
document to find it, or link to a given line number on GitHub and hope
the file doesn't change.

Instead, convert the document to AsciiDoc.  Build it as part of the
technical documentation, since it is likely of interest to the same
group of people.  Provide stable links to the sections which outside
parties are likely to want to link to.  Make some minor structural
changes to organize it so that it can be formatted sanely.

Since the makefile needs a .txt extension in order to build with the
rest of the documentation, simply copy the file.  Ignore the temporary
file so it doesn't get checked in accidentally, and remove it as part of
the clean process.  Do this instead of renaming the file so that people
who have already linked to the documentation (who we're trying to help)
don't find their links broken.  Avoid symlinking since Windows will not
like that.

This allows us to render the document as part of the website for the
benefit of others who wish to link to it as well as providing a more
nicely formatted display for our community and potential contributors.

Signed-off-by: brian m. carlson 
---
 Documentation/.gitignore|   1 +
 Documentation/Makefile  |   5 +
 Documentation/SubmittingPatches | 340 +---
 3 files changed, 188 insertions(+), 158 deletions(-)

diff --git a/Documentation/.gitignore b/Documentation/.gitignore
index 2c8b2d612e..c7096f11f1 100644
--- a/Documentation/.gitignore
+++ b/Documentation/.gitignore
@@ -11,3 +11,4 @@ doc.dep
 cmds-*.txt
 mergetools-*.txt
 manpage-base-url.xsl
+SubmittingPatches.txt
diff --git a/Documentation/Makefile b/Documentation/Makefile
index d5ad85459e..2ab65561af 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -67,6 +67,7 @@ SP_ARTICLES += howto/maintain-git
 API_DOCS = $(patsubst %.txt,%,$(filter-out technical/api-index-skel.txt 
technical/api-index.txt, $(wildcard technical/api-*.txt)))
 SP_ARTICLES += $(API_DOCS)
 
+TECH_DOCS += SubmittingPatches
 TECH_DOCS += technical/hash-function-transition
 TECH_DOCS += technical/http-protocol
 TECH_DOCS += technical/index-format
@@ -324,6 +325,7 @@ clean:
$(RM) *.pdf
$(RM) howto-index.txt howto/*.html doc.dep
$(RM) technical/*.html technical/api-index.txt
+   $(RM) SubmittingPatches.txt
$(RM) $(cmds_txt) $(mergetools_txt) *.made
$(RM) manpage-base-url.xsl
 
@@ -362,6 +364,9 @@ technical/%.html: ASCIIDOC_EXTRA += -a 
git-relative-html-prefix=../
 $(patsubst %,%.html,$(API_DOCS) technical/api-index $(TECH_DOCS)): %.html : 
%.txt asciidoc.conf
$(QUIET_ASCIIDOC)$(TXT_TO_HTML) $*.txt
 
+SubmittingPatches.txt: SubmittingPatches
+   $(QUIET_GEN) cp $< $@
+
 XSLT = docbook.xsl
 XSLTOPTS = --xinclude --stringparam html.stylesheet docbook-xsl.css
 
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 558d465b65..17419f7901 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -1,40 +1,47 @@
+Submitting Patches
+==
+
+== Guidelines
+
 Here are some guidelines for people who want to contribute their code
 to this software.
 
-(0) Decide what to base your work on.
+[[base-branch]]
+=== Decide what to base your work on.
 
 In general, always base your work on the oldest branch that your
 change is relevant to.
 
- - A bugfix should be based on 'maint' in general. If the bug is not
-   present in 'maint', base it on 'master'. For a bug that's not yet
-   in 'master', find the topic that introduces the regression, and
-   base your work on the tip of the topic.
+* A bugfix should be based on `maint` in general. If the bug is not
+  present in `maint`, base it on `master`. For a bug that's not yet
+  in `master`, find the topic that introduces the regression, and
+  base your work on the tip of the topic.
 
- - A new feature should be based on 'master' in general. If the new
-   feature depends on a topic that is in 'pu', but not in 'master',
-   base your work on the tip of that topic.
+* A new feature should be based on `master` in general. If the new
+  feature depends on a topic that is in `pu`, but not in `master`,
+  base your work on the tip of that topic.
 
- - Corrections and enhancements to a topic not yet in 'master' should
-   be based on the tip of that topic. If the topic has not been merged
-   to 'next', it's alright to add a note to squash minor corrections
-   into the series.
+* Corrections and enhancements to a topic not yet in `master` should
+  be based on the tip of that topic. If the topic has not been merged
+  to `next`, it's alright to 

[PATCH v2 1/2] Documentation: enable compat-mode for Asciidoctor

[brian m. carlson]

Asciidoctor 1.5.0 and later have a compatibility mode that makes it more
compatible with some Asciidoc syntax, notably the single and double
quote handling.  While this doesn't affect any of our current
documentation, it would be beneficial to enable this mode to reduce the
differences between AsciiDoc and Asciidoctor if we make use of those
features in the future.

Since this mode is specified as an attribute, if a version of
Asciidoctor doesn't understand it, it will simply be ignored.

Signed-off-by: brian m. carlson 
---
 Documentation/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index 471bb29725..d5ad85459e 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -181,6 +181,7 @@ ASCIIDOC = asciidoctor
 ASCIIDOC_CONF =
 ASCIIDOC_HTML = xhtml5
 ASCIIDOC_DOCBOOK = docbook45
+ASCIIDOC_EXTRA += -acompat-mode
 ASCIIDOC_EXTRA += -I. -rasciidoctor-extensions
 ASCIIDOC_EXTRA += -alitdd='&\#x2d;&\#x2d;'
 DBLATEX_COMMON =

[PATCH v3 1/4] setup: expose enumerated repo info

[brian m. carlson]

We enumerate several different items as part of struct
repository_format, but then actually set up those values using the
global variables we've initialized from them.  Instead, let's pass a
pointer to the structure down to the code where we enumerate these
values, so we can later on use those values directly to perform setup.

This technique makes it easier for us to determine additional items
about the repository format (such as the hash algorithm) and then use
them for setup later on, without needing to add additional global
variables.  We can't avoid using the existing global variables since
they're intricately intertwined with how things work at the moment, but
this improves things for the future.

Signed-off-by: brian m. carlson 
---
 setup.c | 46 +-
 1 file changed, 25 insertions(+), 21 deletions(-)

diff --git a/setup.c b/setup.c
index 94768512b7..cf1b22cd30 100644
--- a/setup.c
+++ b/setup.c
@@ -434,16 +434,15 @@ static int check_repo_format(const char *var, const char 
*value, void *vdata)
return 0;
 }
 
-static int check_repository_format_gently(const char *gitdir, int *nongit_ok)
+static int check_repository_format_gently(const char *gitdir, struct 
repository_format *candidate, int *nongit_ok)
 {
struct strbuf sb = STRBUF_INIT;
struct strbuf err = STRBUF_INIT;
-   struct repository_format candidate;
int has_common;
 
has_common = get_common_dir(, gitdir);
strbuf_addstr(, "/config");
-   read_repository_format(, sb.buf);
+   read_repository_format(candidate, sb.buf);
strbuf_release();
 
/*
@@ -451,10 +450,10 @@ static int check_repository_format_gently(const char 
*gitdir, int *nongit_ok)
 * we treat a missing config as a silent "ok", even when nongit_ok
 * is unset.
 */
-   if (candidate.version < 0)
+   if (candidate->version < 0)
return 0;
 
-   if (verify_repository_format(, ) < 0) {
+   if (verify_repository_format(candidate, ) < 0) {
if (nongit_ok) {
warning("%s", err.buf);
strbuf_release();
@@ -464,21 +463,21 @@ static int check_repository_format_gently(const char 
*gitdir, int *nongit_ok)
die("%s", err.buf);
}
 
-   repository_format_precious_objects = candidate.precious_objects;
-   string_list_clear(_extensions, 0);
+   repository_format_precious_objects = candidate->precious_objects;
+   string_list_clear(>unknown_extensions, 0);
if (!has_common) {
-   if (candidate.is_bare != -1) {
-   is_bare_repository_cfg = candidate.is_bare;
+   if (candidate->is_bare != -1) {
+   is_bare_repository_cfg = candidate->is_bare;
if (is_bare_repository_cfg == 1)
inside_work_tree = -1;
}
-   if (candidate.work_tree) {
+   if (candidate->work_tree) {
free(git_work_tree_cfg);
-   git_work_tree_cfg = candidate.work_tree;
+   git_work_tree_cfg = candidate->work_tree;
inside_work_tree = -1;
}
} else {
-   free(candidate.work_tree);
+   free(candidate->work_tree);
}
 
return 0;
@@ -625,6 +624,7 @@ const char *read_gitfile_gently(const char *path, int 
*return_error_code)
 
 static const char *setup_explicit_git_dir(const char *gitdirenv,
  struct strbuf *cwd,
+ struct repository_format *repo_fmt,
  int *nongit_ok)
 {
const char *work_tree_env = getenv(GIT_WORK_TREE_ENVIRONMENT);
@@ -650,7 +650,7 @@ static const char *setup_explicit_git_dir(const char 
*gitdirenv,
die("Not a git repository: '%s'", gitdirenv);
}
 
-   if (check_repository_format_gently(gitdirenv, nongit_ok)) {
+   if (check_repository_format_gently(gitdirenv, repo_fmt, nongit_ok)) {
free(gitfile);
return NULL;
}
@@ -723,9 +723,10 @@ static const char *setup_explicit_git_dir(const char 
*gitdirenv,
 
 static const char *setup_discovered_git_dir(const char *gitdir,
struct strbuf *cwd, int offset,
+   struct repository_format *repo_fmt,
int *nongit_ok)
 {
-   if (check_repository_format_gently(gitdir, nongit_ok))
+   if (check_repository_format_gently(gitdir, repo_fmt, nongit_ok))
return NULL;
 
/* --work-tree is set without --git-dir; use discovered one */
@@ -737,7 +738,7 @@ static const char *setup_discovered_git_dir(const char 
*gitdir,
gitdir = 

[PATCH v3 2/4] Add structure representing hash algorithm

[brian m. carlson]

Since in the future we want to support an additional hash algorithm, add
a structure that represents a hash algorithm and all the data that must
go along with it.  Add a constant to allow easy enumeration of hash
algorithms.  Implement function typedefs to create an abstract API that
can be used by any hash algorithm, and wrappers for the existing SHA1
functions that conform to this API.

Expose a value for hex size as well as binary size.  While one will
always be twice the other, the two values are both used extremely
commonly throughout the codebase and providing both leads to improved
readability.

Don't include an entry in the hash algorithm structure for the null
object ID.  As this value is all zeros, any suitably sized all-zero
object ID can be used, and there's no need to store a given one on a
per-hash basis.

The current hash function transition plan envisions a time when we will
accept input from the user that might be in SHA-1 or in the NewHash
format.  Since we cannot know which the user has provided, add a
constant representing the unknown algorithm to allow us to indicate that
we must look the correct value up.  Provide dummy API functions that die
in this case.

Finally, include git-compat-util.h in hash.h so that the required types
are available.  This aids people using automated tools their editors.

Signed-off-by: brian m. carlson 
---
 hash.h  | 57 +
 sha1_file.c | 58 ++
 2 files changed, 115 insertions(+)

diff --git a/hash.h b/hash.h
index 024d0d3d50..7d7a864f5d 100644
--- a/hash.h
+++ b/hash.h
@@ -1,6 +1,8 @@
 #ifndef HASH_H
 #define HASH_H
 
+#include "git-compat-util.h"
+
 #if defined(SHA1_PPC)
 #include "ppc/sha1.h"
 #elif defined(SHA1_APPLE)
@@ -13,4 +15,59 @@
 #include "block-sha1/sha1.h"
 #endif
 
+/*
+ * Note that these constants are suitable for indexing the hash_algos array and
+ * comparing against each other, but are otherwise arbitrary, so they should 
not
+ * be exposed to the user or serialized to disk.  To know whether a
+ * git_hash_algo struct points to some usable hash function, test the format_id
+ * field for being non-zero.  Use the name field for user-visible situations 
and
+ * the format_id field for fixed-length fields on disk.
+ */
+/* An unknown hash function. */
+#define GIT_HASH_UNKNOWN 0
+/* SHA-1 */
+#define GIT_HASH_SHA1 1
+/* Number of algorithms supported (including unknown). */
+#define GIT_HASH_NALGOS (GIT_HASH_SHA1 + 1)
+
+typedef void (*git_hash_init_fn)(void *ctx);
+typedef void (*git_hash_update_fn)(void *ctx, const void *in, size_t len);
+typedef void (*git_hash_final_fn)(unsigned char *hash, void *ctx);
+
+struct git_hash_algo {
+   /*
+* The name of the algorithm, as appears in the config file and in
+* messages.
+*/
+   const char *name;
+
+   /* A four-byte version identifier, used in pack indices. */
+   uint32_t format_id;
+
+   /* The size of a hash context (e.g. git_SHA_CTX). */
+   size_t ctxsz;
+
+   /* The length of the hash in binary. */
+   size_t rawsz;
+
+   /* The length of the hash in hex characters. */
+   size_t hexsz;
+
+   /* The hash initialization function. */
+   git_hash_init_fn init_fn;
+
+   /* The hash update function. */
+   git_hash_update_fn update_fn;
+
+   /* The hash finalization function. */
+   git_hash_final_fn final_fn;
+
+   /* The OID of the empty tree. */
+   const struct object_id *empty_tree;
+
+   /* The OID of the empty blob. */
+   const struct object_id *empty_blob;
+};
+extern const struct git_hash_algo hash_algos[GIT_HASH_NALGOS];
+
 #endif
diff --git a/sha1_file.c b/sha1_file.c
index d708981376..a04389be71 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -39,6 +39,64 @@ const struct object_id empty_blob_oid = {
EMPTY_BLOB_SHA1_BIN_LITERAL
 };
 
+static void git_hash_sha1_init(void *ctx)
+{
+   git_SHA1_Init((git_SHA_CTX *)ctx);
+}
+
+static void git_hash_sha1_update(void *ctx, const void *data, size_t len)
+{
+   git_SHA1_Update((git_SHA_CTX *)ctx, data, len);
+}
+
+static void git_hash_sha1_final(unsigned char *hash, void *ctx)
+{
+   git_SHA1_Final(hash, (git_SHA_CTX *)ctx);
+}
+
+static void git_hash_unknown_init(void *ctx)
+{
+   die("trying to init unknown hash");
+}
+
+static void git_hash_unknown_update(void *ctx, const void *data, size_t len)
+{
+   die("trying to update unknown hash");
+}
+
+static void git_hash_unknown_final(unsigned char *hash, void *ctx)
+{
+   die("trying to finalize unknown hash");
+}
+
+const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {
+   {
+   NULL,
+   0x,
+   0,
+   0,
+   0,
+   git_hash_unknown_init,
+   git_hash_unknown_update,
+   git_hash_unknown_final,
+  

[PATCH v3 3/4] Integrate hash algorithm support with repo setup

[brian m. carlson]

In future versions of Git, we plan to support an additional hash
algorithm.  Integrate the enumeration of hash algorithms with repository
setup, and store a pointer to the enumerated data in struct repository.
Of course, we currently only support SHA-1, so hard-code this value in
read_repository_format.  In the future, we'll enumerate this value from
the configuration.

Add a constant, the_hash_algo, which points to the hash_algo structure
pointer in the repository global.  Note that this is the hash which is
used to serialize data to disk, not the hash which is used to display
items to the user.  The transition plan anticipates that these may be
different.  We can add an additional element in the future (say,
ui_hash_algo) to provide for this case.

Include repository.h in cache.h since we now need to have access to
these struct and variable definitions.

Signed-off-by: brian m. carlson 
---
 cache.h  | 4 
 repository.c | 7 +++
 repository.h | 5 +
 setup.c  | 3 +++
 4 files changed, 19 insertions(+)

diff --git a/cache.h b/cache.h
index cb7fb7c004..c238688f6c 100644
--- a/cache.h
+++ b/cache.h
@@ -14,6 +14,7 @@
 #include "hash.h"
 #include "path.h"
 #include "sha1-array.h"
+#include "repository.h"
 
 #ifndef platform_SHA_CTX
 /*
@@ -77,6 +78,8 @@ struct object_id {
unsigned char hash[GIT_MAX_RAWSZ];
 };
 
+#define the_hash_algo the_repository->hash_algo
+
 #if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)
 #define DTYPE(de)  ((de)->d_type)
 #else
@@ -888,6 +891,7 @@ struct repository_format {
int version;
int precious_objects;
int is_bare;
+   int hash_algo;
char *work_tree;
struct string_list unknown_extensions;
 };
diff --git a/repository.c b/repository.c
index bb2fae5446..c6ceb9f9e4 100644
--- a/repository.c
+++ b/repository.c
@@ -64,6 +64,11 @@ void repo_set_gitdir(struct repository *repo, const char 
*path)
free(old_gitdir);
 }
 
+void repo_set_hash_algo(struct repository *repo, int hash_algo)
+{
+   repo->hash_algo = _algos[hash_algo];
+}
+
 /*
  * Attempt to resolve and set the provided 'gitdir' for repository 'repo'.
  * Return 0 upon success and a non-zero value upon failure.
@@ -136,6 +141,8 @@ int repo_init(struct repository *repo, const char *gitdir, 
const char *worktree)
if (read_and_verify_repository_format(, repo->commondir))
goto error;
 
+   repo_set_hash_algo(repo, format.hash_algo);
+
if (worktree)
repo_set_worktree(repo, worktree);
 
diff --git a/repository.h b/repository.h
index 7f5e24a0a2..0329e40c7f 100644
--- a/repository.h
+++ b/repository.h
@@ -4,6 +4,7 @@
 struct config_set;
 struct index_state;
 struct submodule_cache;
+struct git_hash_algo;
 
 struct repository {
/* Environment */
@@ -67,6 +68,9 @@ struct repository {
 */
struct index_state *index;
 
+   /* Repository's current hash algorithm, as serialized on disk. */
+   const struct git_hash_algo *hash_algo;
+
/* Configurations */
/*
 * Bit used during initialization to indicate if repository state (like
@@ -86,6 +90,7 @@ extern struct repository *the_repository;
 
 extern void repo_set_gitdir(struct repository *repo, const char *path);
 extern void repo_set_worktree(struct repository *repo, const char *path);
+extern void repo_set_hash_algo(struct repository *repo, int algo);
 extern int repo_init(struct repository *repo, const char *gitdir, const char 
*worktree);
 extern int repo_submodule_init(struct repository *submodule,
   struct repository *superproject,
diff --git a/setup.c b/setup.c
index cf1b22cd30..50c6b2ab11 100644
--- a/setup.c
+++ b/setup.c
@@ -488,6 +488,7 @@ int read_repository_format(struct repository_format 
*format, const char *path)
memset(format, 0, sizeof(*format));
format->version = -1;
format->is_bare = -1;
+   format->hash_algo = GIT_HASH_SHA1;
string_list_init(>unknown_extensions, 1);
git_config_from_file(check_repo_format, path, format);
return format->version;
@@ -1113,6 +1114,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
repo_set_gitdir(the_repository, gitdir);
setup_git_env();
}
+   if (startup_info->have_repository)
+   repo_set_hash_algo(the_repository, repo_fmt.hash_algo);
}
 
strbuf_release();

[PATCH v3 4/4] Switch empty tree and blob lookups to use hash abstraction

[brian m. carlson]

Switch the uses of empty_tree_oid and empty_blob_oid to use the
current_hash abstraction that represents the current hash algorithm in
use.

Signed-off-by: brian m. carlson 
---
 builtin/am.c   | 2 +-
 builtin/checkout.c | 2 +-
 builtin/diff.c | 2 +-
 builtin/pull.c | 2 +-
 cache.h| 8 
 diff-lib.c | 2 +-
 merge-recursive.c  | 2 +-
 notes-merge.c  | 2 +-
 sequencer.c| 6 +++---
 submodule.c| 2 +-
 10 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 92c4853505..99dbde3e85 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1433,7 +1433,7 @@ static void write_index_patch(const struct am_state 
*state)
if (!get_oid_tree("HEAD", ))
tree = lookup_tree();
else
-   tree = lookup_tree(_tree_oid);
+   tree = lookup_tree(the_hash_algo->empty_tree);
 
fp = xfopen(am_path(state, "patch"), "w");
init_revisions(_info, NULL);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 6c2b4cd419..2b64805f35 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -514,7 +514,7 @@ static int merge_working_tree(const struct checkout_opts 
*opts,
}
tree = parse_tree_indirect(old->commit ?
   >commit->object.oid :
-  _tree_oid);
+  the_hash_algo->empty_tree);
init_tree_desc([0], tree->buffer, tree->size);
tree = parse_tree_indirect(>commit->object.oid);
init_tree_desc([1], tree->buffer, tree->size);
diff --git a/builtin/diff.c b/builtin/diff.c
index 9808d062a8..16bfb22f73 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -379,7 +379,7 @@ int cmd_diff(int argc, const char **argv, const char 
*prefix)
add_head_to_pending();
if (!rev.pending.nr) {
struct tree *tree;
-   tree = lookup_tree(_tree_oid);
+   tree = 
lookup_tree(the_hash_algo->empty_tree);
add_pending_object(, >object, 
"HEAD");
}
break;
diff --git a/builtin/pull.c b/builtin/pull.c
index a28f0ffadd..3d26f8ff32 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -545,7 +545,7 @@ static int pull_into_void(const struct object_id 
*merge_head,
 * index/worktree changes that the user already made on the unborn
 * branch.
 */
-   if (checkout_fast_forward(_tree_oid, merge_head, 0))
+   if (checkout_fast_forward(the_hash_algo->empty_tree, merge_head, 0))
return 1;
 
if (update_ref("initial pull", "HEAD", merge_head, curr_head, 0, 
UPDATE_REFS_DIE_ON_ERR))
diff --git a/cache.h b/cache.h
index c238688f6c..d68895b45f 100644
--- a/cache.h
+++ b/cache.h
@@ -1024,22 +1024,22 @@ extern const struct object_id empty_blob_oid;
 
 static inline int is_empty_blob_sha1(const unsigned char *sha1)
 {
-   return !hashcmp(sha1, EMPTY_BLOB_SHA1_BIN);
+   return !hashcmp(sha1, the_hash_algo->empty_blob->hash);
 }
 
 static inline int is_empty_blob_oid(const struct object_id *oid)
 {
-   return !hashcmp(oid->hash, EMPTY_BLOB_SHA1_BIN);
+   return !oidcmp(oid, the_hash_algo->empty_blob);
 }
 
 static inline int is_empty_tree_sha1(const unsigned char *sha1)
 {
-   return !hashcmp(sha1, EMPTY_TREE_SHA1_BIN);
+   return !hashcmp(sha1, the_hash_algo->empty_tree->hash);
 }
 
 static inline int is_empty_tree_oid(const struct object_id *oid)
 {
-   return !hashcmp(oid->hash, EMPTY_TREE_SHA1_BIN);
+   return !oidcmp(oid, the_hash_algo->empty_tree);
 }
 
 /* set default permissions by passing mode arguments to open(2) */
diff --git a/diff-lib.c b/diff-lib.c
index 731f0886d6..893fee432c 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -217,7 +217,7 @@ int run_diff_files(struct rev_info *revs, unsigned int 
option)
} else if (revs->diffopt.ita_invisible_in_index &&
   ce_intent_to_add(ce)) {
diff_addremove(>diffopt, '+', ce->ce_mode,
-  _tree_oid, 0,
+  the_hash_algo->empty_tree, 0,
   ce->name, 0);
continue;
}
diff --git a/merge-recursive.c b/merge-recursive.c
index 2ca8444c65..f89cc751e1 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2081,7 +2081,7 @@ int merge_recursive(struct merge_options *o,
/* if there is no common ancestor, use an empty tree */
struct tree *tree;
 
-   tree = lookup_tree(_tree_oid);
+ 

[PATCH v3 0/4] Hash abstraction

[brian m. carlson]

This is a series proposing a basic abstraction for hash functions.

The full description of the series can be found here:
https://public-inbox.org/git/20171028181239.59458-1-sand...@crustytoothpaste.net/

At the bottom of this message is the output of git tbdiff between v2 and
v3.

Changes from v2:
* Remove inline.
* Add dummy functions that call die for unknown hash implementation.
* Move structure definitions to hash.h and include git-compat-util.h in
  hash.h.
* Rename current_hash to the_hash_algo.
* Use repo_set_hash_algo everywhere we set the hash algorithm for a
  struct repository.

Changes from v1:
* Rebase onto 2.15-rc2.
* Fix the uninitialized value that Peff pointed out.  This fixes the
  error, but leaves the code in the same place, since I think it's where
  it should be.
* Improve commit message to explain the meaning of current_hash WRT the
  transition plan.
* Added an unknown hash algorithm constant and value to better implement
  the transition plan.
* Explain in the commit message why hex size and binary size are both
  provided.
* Add a format_id field to the struct, in coordination with the
  transition plan.
* Improve comments for struct fields and constants.

brian m. carlson (4):
  setup: expose enumerated repo info
  Add structure representing hash algorithm
  Integrate hash algorithm support with repo setup
  Switch empty tree and blob lookups to use hash abstraction

 builtin/am.c   |  2 +-
 builtin/checkout.c |  2 +-
 builtin/diff.c |  2 +-
 builtin/pull.c |  2 +-
 cache.h| 12 +++
 diff-lib.c |  2 +-
 hash.h | 57 +
 merge-recursive.c  |  2 +-
 notes-merge.c  |  2 +-
 repository.c   |  7 +++
 repository.h   |  5 +
 sequencer.c|  6 +++---
 setup.c| 49 +
 sha1_file.c| 58 ++
 submodule.c|  2 +-
 15 files changed, 174 insertions(+), 36 deletions(-)

git-tbdiff output:

1: 7de9bc96ff = 1: 1ec859c3a8 setup: expose enumerated repo info
2: fb5099972b ! 2: c9c628b745 Add structure representing hash algorithm
@@ -23,16 +23,29 @@
 accept input from the user that might be in SHA-1 or in the NewHash
 format.  Since we cannot know which the user has provided, add a
 constant representing the unknown algorithm to allow us to indicate 
that
-we must look the correct value up.
+we must look the correct value up.  Provide dummy API functions that 
die
+in this case.
+
+Finally, include git-compat-util.h in hash.h so that the required types
+are available.  This aids people using automated tools their editors.
 
 Signed-off-by: brian m. carlson 
 
-diff --git a/cache.h b/cache.h
 a/cache.h
-+++ b/cache.h
+diff --git a/hash.h b/hash.h
+--- a/hash.h
 b/hash.h
 @@
-   unsigned char hash[GIT_MAX_RAWSZ];
- };
+ #ifndef HASH_H
+ #define HASH_H
+ 
++#include "git-compat-util.h"
++
+ #if defined(SHA1_PPC)
+ #include "ppc/sha1.h"
+ #elif defined(SHA1_APPLE)
+@@
+ #include "block-sha1/sha1.h"
+ #endif
  
 +/*
 + * Note that these constants are suitable for indexing the hash_algos 
array and
@@ -89,9 +102,7 @@
 +};
 +extern const struct git_hash_algo hash_algos[GIT_HASH_NALGOS];
 +
- #if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)
- #define DTYPE(de) ((de)->d_type)
- #else
+ #endif
 
 diff --git a/sha1_file.c b/sha1_file.c
 --- a/sha1_file.c
@@ -100,19 +111,34 @@
EMPTY_BLOB_SHA1_BIN_LITERAL
  };
  
-+static inline void git_hash_sha1_init(void *ctx)
++static void git_hash_sha1_init(void *ctx)
 +{
 +  git_SHA1_Init((git_SHA_CTX *)ctx);
 +}
 +
-+static inline void git_hash_sha1_update(void *ctx, const void *data, 
size_t len)
++static void git_hash_sha1_update(void *ctx, const void *data, size_t len)
 +{
 +  git_SHA1_Update((git_SHA_CTX *)ctx, data, len);
 +}
 +
-+static inline void git_hash_sha1_final(unsigned char *hash, void *ctx)
++static void git_hash_sha1_final(unsigned char *hash, void *ctx)
 +{
 +  git_SHA1_Final(hash, (git_SHA_CTX *)ctx);
++}
++
++static void git_hash_unknown_init(void *ctx)
++{
++  die("trying to init unknown hash");
++}
++
++static void git_hash_unknown_update(void *ctx, const void *data, size_t 
len)
++{
++  die("trying to update unknown hash");
++}
++
++static void git_hash_unknown_final(unsigned char *hash, void *ctx)
++{
++  die("trying to finalize unknown hash");
 +}
 +
 +const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {
@@ -122,9 +148,9 @@
 +  0,
 +  0,
 +

Re: "git bisect" takes exactly one bad commit and one or more good?

[Stephan Beyer]

On 11/11/2017 03:34 PM, Junio C Hamano wrote:
> Christian Couder  writes:
> 
>>> "You use it by first telling it a "bad" commit that is known to
>>> contain the bug, and a "good" commit that is known to be before the
>>> bug was introduced."
>>
>> Yeah, 'and at least a "good" commit' would be better.
> 
> Make it "at least one" instead, perhaps?
> 
> I somehow thought that you technically could force bisection with 0
> good commit, even though no sane person would do so.

Thanks for pointing that out but I disagree with the part after "even
though" :)

Imagine you add a test case that was totally uncovered before and now
reveals a bug. You want to find the introduction of the bug, so you can
either check out the first commit you think where that bug did not
exist, then you find out that its also a bad commit, so you check out
another commit... essentially you are manually doing a "bisect" but less
efficient. So it would be better to let "git bisect" do its job without
knowing a good commit in advance. Sounds perfectly sane to me.

The probably insane thing is that there are currently performance issues
with git bisect. So you *are* probably faster by guessing. But that is
what my patch series [1] was about (and that I postponed in favor of
other conflicting work on bisect).

1.
https://public-inbox.org/git/1460294354-7031-1-git-send-email-s-be...@gmx.net/

Cheers
Stephan

[PATCH] doc: Remove explanation of "--" from several man pages

[Robert P. J. Day]

There is no value in individual man pages explaining the purpose of
the "--" separator.

Signed-off-by: Robert P. J. Day 

---

  unless every man page explains that option, it's pointless to have
just *some* man pages explaining it, so might as well remove it from
all of them.

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index b700beaff..69d625285 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -180,11 +180,6 @@ for "git add --no-all ...", i.e. ignored removed 
files.
bit is only changed in the index, the files on disk are left
unchanged.

-\--::
-   This option can be used to separate command-line options from
-   the list of files, (useful when filenames might be mistaken
-   for command-line options).
-

 Configuration
 -
diff --git a/Documentation/git-check-attr.txt b/Documentation/git-check-attr.txt
index aa3b2bf2f..0ae2523e0 100644
--- a/Documentation/git-check-attr.txt
+++ b/Documentation/git-check-attr.txt
@@ -36,10 +36,6 @@ OPTIONS
If `--stdin` is also given, input paths are separated
with a NUL character instead of a linefeed character.

-\--::
-   Interpret all preceding arguments as attributes and all following
-   arguments as path names.
-
 If none of `--stdin`, `--all`, or `--` is used, the first argument
 will be treated as an attribute and the rest of the arguments as
 pathnames.
diff --git a/Documentation/git-checkout-index.txt 
b/Documentation/git-checkout-index.txt
index 4d33e7be0..11ee76e7d 100644
--- a/Documentation/git-checkout-index.txt
+++ b/Documentation/git-checkout-index.txt
@@ -68,9 +68,6 @@ OPTIONS
Only meaningful with `--stdin`; paths are separated with
NUL character instead of LF.

-\--::
-   Do not interpret any more arguments as options.
-
 The order of the flags used to matter, but not anymore.

 Just doing `git checkout-index` does nothing. You probably meant
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 8c74a2ca0..cd9f362d1 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -334,9 +334,6 @@ changes to tracked files.
Countermand `commit.gpgSign` configuration variable that is
set to force each and every commit to be signed.

-\--::
-   Do not interpret any more arguments as options.
-
 ...::
When files are given on the command line, the command
commits the contents of the named files, without
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 18b494731..bac0b789c 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -282,10 +282,6 @@ providing this option will cause it to die.
Instead of searching tracked files in the working tree, search
blobs in the given trees.

-\--::
-   Signals the end of options; the rest of the parameters
-   are  limiters.
-
 ...::
If given, limit the search to paths matching at least one pattern.
Both leading paths match and glob(7) patterns are supported.
diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index d153c17e0..93ebb020c 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -171,9 +171,6 @@ Both the  in the index ("i/")
 and in the working tree ("w/") are shown for regular files,
 followed by the  ("attr/").

-\--::
-   Do not interpret any more arguments as options.
-
 ::
Files to show. If no files are given all files which match the other
specified criteria are shown.
diff --git a/Documentation/git-merge-index.txt 
b/Documentation/git-merge-index.txt
index 02676fb39..51f884c7e 100644
--- a/Documentation/git-merge-index.txt
+++ b/Documentation/git-merge-index.txt
@@ -20,9 +20,6 @@ files are passed as arguments 5, 6 and 7.

 OPTIONS
 ---
-\--::
-   Do not interpret any more arguments as options.
-
 -a::
Run merge against all files in the index that need merging.

diff --git a/Documentation/git-prune.txt b/Documentation/git-prune.txt
index 7a493c80f..39caa247a 100644
--- a/Documentation/git-prune.txt
+++ b/Documentation/git-prune.txt
@@ -42,9 +42,6 @@ OPTIONS
 --verbose::
Report all removed objects.

-\--::
-   Do not interpret any more arguments as options.
-
 --expire ::
Only expire loose objects older than .

diff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt
index b5c46223c..67ea38e46 100644
--- a/Documentation/git-rm.txt
+++ b/Documentation/git-rm.txt
@@ -50,11 +50,6 @@ OPTIONS
 Allow recursive removal when a leading directory name is
 given.

-\--::
-   This option can be used to separate command-line options from
-   the list of files, (useful when filenames might be mistaken
-   for command-line options).
-
 --cached::
Use this option to unstage and remove paths only from the index.
Working tree files, whether modified or 

[PATCH] bisect run: die if no command is given

[Stephan Beyer]

It was possible to invoke "git bisect run" without any command.
This considers all commits as good commits since "$@"'s return
value for empty $@ is 0.

This is most probably not what a user wants (otherwise she would
invoke "git bisect run true"), so not providing a command now
results in an error.

Signed-off-by: Stephan Beyer 
---
 git-bisect.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/git-bisect.sh b/git-bisect.sh
index 0138a8860..a69e43656 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -450,6 +450,8 @@ bisect_replay () {
 bisect_run () {
bisect_next_check fail
 
+   test -n "$*" || die "$(gettext "bisect run failed: no command 
provided.")"
+
while true
do
command="$@"
-- 
2.15.0.165.g0dc13a7db.dirty

aesthetic standard for synopsis line of man pages?

[Robert P. J. Day]

  yet more aesthetic nitpickery ... was just perusing the man pages of
both "git clean" and "git rm", and noticed some striking
inconsistency.

  from "man git-clean":

  SYNOPSIS
   git clean [-d] [-f] [-i] [-n] [-q] [-e ] [-x | -X] [--] 
...

note how, in that sypnosis, even those options that are represented by
both a short form option (-f) and a corresponding long form (--force)
use only the short form in the synopsis, i'm assuming for brevity,
which makes perfect sense.

  "man git-rm" is a different beast entirely, with a hodge podge of
short forms and long forms with no apparent pattern:

  SYNOPSIS
   git rm [-f | --force] [-n] [-r] [--cached] [--ignore-unmatch] [--quiet] 
[--] ...

  ... snip ...

  OPTIONS

   -f, --force
   Override the up-to-date check.

   -n, --dry-run
   Don’t actually remove any file(s). Instead, just show if
   they exist in the index and would otherwise be removed by
   the command.

   -r
   Allow recursive removal when a leading directory name is
   given.

   --
   This option can be used to separate command-line options
   from the list of files, (useful when filenames might be
   mistaken for command-line options).

   --cached
   Use this option to unstage and remove paths only from the
   index. Working tree files, whether modified or not, will
   be left alone.

   --ignore-unmatch
   Exit with a zero status even if no files matched.

   -q, --quiet
   git rm normally outputs one line (in the form of an rm
   command) for each file removed. This option suppresses
   that output.


  the strangeness above?

1) the synopsis itself lists the alternatives "[-f | --force]", which
seems unnecessary, as both forms are listed under OPTIONS

2) this is followed by *only* the short form (-n) of --dry-run, so
that's inconsistent

3) the SYNOPSIS weirdly includes *only* the long form (--quiet) rather
than the short form (-q)

4) is it standard to explain the "--" separator in a man page? i
don't recall seeing that in any other man page, but maybe i wasn't
paying attention. it seems unnecessary.

  in short, "man git-clean" seems reasonable, while "man git-rm"
appears somewhat disorganized.

rday

-- 


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

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

Re: [PATCH] Fix NO_LIBPCRE1_JIT to fully disable JIT

[Ævar Arnfjörð Bjarmason]

On Sun, Nov 12 2017, Charles Bailey jotted:

> From: Charles Bailey 
>
> If you have a pcre1 library which is compiled with JIT enabled then
> PCRE_STUDY_JIT_COMPILE will be defined whether or not the
> NO_LIBPCRE1_JIT configuration is set.
>
> This means that we enable JIT functionality when calling pcre_study
> even if NO_LIBPCRE1_JIT has been explicitly set and we just use plain
> pcre_exec later.
>
> Fix this by using own macro (GIT_PCRE_STUDY_JIT_COMPILE) which we set to
> PCRE_STUDY_JIT_COMPILE only if NO_LIBPCRE1_JIT is not set and define to
> 0 otherwise, as before.
> ---
>
> I was bisecting an issue with the PCRE support that was causing a test
> suite failure on our Solaris builds and reached fbaceaac47 ("grep: add
> support for the PCRE v1 JIT API"). It appeared to be a misaligned memory
> access somewhere inside the libpcre code. I tried disabling the use of
> JIT with NO_LIBPCRE1_JIT but it turned out that even with this set we
> were still triggering the JIT code path in the call to pcre_study.
>
> Yes, we probably should fix our PCRE1 library build on Solaris or move
> to PCRE2, but really NO_LIBPCRE1_JIT should have prevented us from
> triggering this crash.
>
>  grep.c | 2 +-
>  grep.h | 5 +++--
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/grep.c b/grep.c
> index ce6a48e..d0b9b6c 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -387,7 +387,7 @@ static void compile_pcre1_regexp(struct grep_pat *p, 
> const struct grep_opt *opt)
>   if (!p->pcre1_regexp)
>   compile_regexp_failed(p, error);
>
> - p->pcre1_extra_info = pcre_study(p->pcre1_regexp, 
> PCRE_STUDY_JIT_COMPILE, );
> + p->pcre1_extra_info = pcre_study(p->pcre1_regexp, 
> GIT_PCRE_STUDY_JIT_COMPILE, );
>   if (!p->pcre1_extra_info && error)
>   die("%s", error);
>
> diff --git a/grep.h b/grep.h
> index 52aecfa..399381c 100644
> --- a/grep.h
> +++ b/grep.h
> @@ -7,11 +7,12 @@
>  #if PCRE_MAJOR >= 8 && PCRE_MINOR >= 32
>  #ifndef NO_LIBPCRE1_JIT
>  #define GIT_PCRE1_USE_JIT
> +#define GIT_PCRE_STUDY_JIT_COMPILE PCRE_STUDY_JIT_COMPILE
>  #endif
>  #endif
>  #endif
> -#ifndef PCRE_STUDY_JIT_COMPILE
> -#define PCRE_STUDY_JIT_COMPILE 0
> +#ifndef GIT_PCRE_STUDY_JIT_COMPILE
> +#define GIT_PCRE_STUDY_JIT_COMPILE 0
>  #endif
>  #if PCRE_MAJOR <= 8 && PCRE_MINOR < 20
>  typedef int pcre_jit_stack;

[CC-ing Junio]

Thanks a lot. This patch looks good to me.

I could have sworn I was handling this already, but looking at this now
I wasn't really.

However, as a bit of extra info I *did* test this, and it works just
fine for me, i.e. if I compile PCRE 8.32 now (as I did at the time)
--without-jit it'll error with just USE_LIBPCRE=YesPlease as expected,
but add NO_LIBPCRE1_JIT=UnfortunatelyYes and it works just fine without
your patch.

However, as your patch shows (and as I've independently verified)
PCRE_STUDY_JIT_COMPILE will still be defined in that case, since PCRE
will be exposing the same headers. This is the logic error in my initial
patch.

*But* for some reason you still get away with that on Linux. I don't
know why, but I assume the compiler toolchain is more lax for some
reason than on Solaris.

All of which is a roundabout way of saying that we should apply this
patch, but that I still have no idea why this worked on Linux before ,
but it does.

But that we should take it anyway regardless of that since it'll *also*
work on Linux with your patch, and this logic makes some sense whereas
the other one clearly didn't and just worked by pure accident of some
toolchain semantics that I haven't figured out yet.

Re: [PATCH v2] gpg-interface: strip CR chars for Windows gpg2

[Torsten Bögershausen]

On Sun, Nov 12, 2017 at 01:07:10PM +, Jerzy Kozera wrote:
> This fixes the issue with newlines being \r\n and not being displayed
> correctly when using gpg2 for Windows, as reported at
> https://github.com/git-for-windows/git/issues/1249
> 
> Issues with non-ASCII characters remain for further investigation.
> 
> Signed-off-by: Jerzy Kozera 
> ---
> 
> Addressed comments by Junio C Hamano (check for following \n, and
> updated the commit description).
> 
>  gpg-interface.c | 27 ++-
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/gpg-interface.c b/gpg-interface.c
> index 4feacf16e5..ab592af7f2 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -145,6 +145,20 @@ const char *get_signing_key(void)
>   return git_committer_info(IDENT_STRICT|IDENT_NO_DATE);
>  }
>  
> +/* Strip CR from the CRLF line endings, in case we are on Windows. */
> +static void strip_cr(struct strbuf *buffer, size_t bottom) {
> + size_t i, j;

It is not wrong to say "Strip CR from the CRLF".
In Git we often talk about "convert CRLF into LF",

The comment somewhat different to the function name.
The function namd and the name of the parameters can be more in
in line with existing strbuf functions:
(And the opening '{' should go into it's own line:

static void convert_crlf_to_lf(struct strbuf *sb, size_t len)
{
size_t i, j;

An even more generic approach (could be done in a seperate commit)
would be to move the whole function into strbuf.c/strbuf.h,
and it may be called like this.

void strbuf_crlf_to_lf(struct strbuf *sb, size_t len)
{
  /* I would even avoid "i" and "j", and use src and dst or so) */
  size_t src_pos, dst_idx;
}

Thanks for working on this.

[]

Re: should "git bisect" support "git bisect next?"

[Stephan Beyer]

Hi,

On 11/11/2017 03:38 PM, Junio C Hamano wrote:
> Christian Couder  writes:
> 
>> On Sat, Nov 11, 2017 at 12:42 PM, Robert P. J. Day
>>  wrote:
>>>
>>>   the man page for "git bisect" makes no mention of "git bisect next",
>>> but the script git-bisect.sh does:
>>
>> Yeah the following patch was related:
>>
>> https://public-inbox.org/git/1460294354-7031-2-git-send-email-s-be...@gmx.net/
>>
>> You might want to discuss with Stephan (cc'ed).
> 
> Thanks for saving me time to explain why 'next' is still a very
> important command but the end users do not actually need to be
> strongly aware of it, because most commands automatically invokes it
> as their final step due to the importance of what it does ;-)

I will nonetheless re-roll the patch (that Christian linked to)
after Pranit's bisect part II series is in good shape. I think the
documentation change in the patch shows why the user should be aware of
it (although not strongly).

Best
Stephan

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

[Torsten Bögershausen]

On Fri, Nov 10, 2017 at 09:22:10AM +0900, Junio C Hamano wrote:

> 
> Taking these altogether, perhaps
> 
> Apply the "clean" process freshly to all tracked files to
> 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`.
> 
> Thanks.

OK with the text - I can send a new version the next days.

Re: [PATCH v16 Part II 5/8] bisect--helper: `bisect_next_check` shell function in C

[Stephan Beyer]

Hi again ;)

On 10/27/2017 05:06 PM, Pranit Bauva wrote:
> @@ -44,6 +46,11 @@ static void set_terms(struct bisect_terms *terms, const 
> char *bad,
>   terms->term_bad = xstrdup(bad);
>  }
>  
> +static const char *voc[] = {
> + "bad|new",
> + "good|old"
> +};
> +
>  /*
>   * Check whether the string `term` belongs to the set of strings
>   * included in the variable arguments.
> @@ -264,6 +271,79 @@ static int check_and_set_terms(struct bisect_terms 
> *terms, const char *cmd)
>   return 0;
>  }
>  
> +static int mark_good(const char *refname, const struct object_id *oid,
> +  int flag, void *cb_data)
> +{
> + int *m_good = (int *)cb_data;
> + *m_good = 0;
> + return 1;
> +}
> +
> +static int bisect_next_check(const struct bisect_terms *terms,
> +  const char *current_term)
> +{
> + int missing_good = 1, missing_bad = 1, retval = 0;
> + const char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
> + const char *good_glob = xstrfmt("%s-*", terms->term_good);
> +
> + if (ref_exists(bad_ref))
> + missing_bad = 0;
> +
> + for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/",
> +  (void *) _good);
> +
> + if (!missing_good && !missing_bad)
> + goto finish;
> +
> + if (!current_term)
> + goto fail;
> +
> + if (missing_good && !missing_bad && current_term &&
> + !strcmp(current_term, terms->term_good)) {
> + char *yesno;
> + /*
> +  * have bad (or new) but not good (or old). We could bisect
> +  * although this is less optimum.
> +  */
> + fprintf(stderr, _("Warning: bisecting only with a %s commit\n"),
> + terms->term_bad);
> + if (!isatty(0))
> + goto finish;
> + /*
> +  * TRANSLATORS: Make sure to include [Y] and [n] in your
> +  * translation. The program will only accept English input
> +  * at this point.
> +  */
> + yesno = git_prompt(_("Are you sure [Y/n]? "), PROMPT_ECHO);
> + if (starts_with(yesno, "N") || starts_with(yesno, "n"))
> + goto fail;
> +
> + goto finish;
> + }
> + if (!is_empty_or_missing_file(git_path_bisect_start())) {
> + error(_("You need to give me at least one %s and "
> + "%s revision. You can use \"git bisect %s\" "
> + "and \"git bisect %s\" for that.\n"),
> + voc[0], voc[1], voc[0], voc[1]);
> + goto fail;
> + } else {
> + error(_("You need to start by \"git bisect start\". You "
> + "then need to give me at least one %s and %s "
> + "revision. You can use \"git bisect %s\" and "
> + "\"git bisect %s\" for that.\n"),
> + voc[1], voc[0], voc[1], voc[0]);
> + goto fail;

In both of the above "error" calls, you should drop the final "\n"
because "error" does that already.

On the other hand, you have dropped the "\n"s of the orginal error
messages. So it should probably be

 _("You need to give me at least one %s and %s revision.\n"
   "You can use \"git bisect %s\" and \"git bisect %s\" for that.")

and

 _("You need to start by \"git bisect start\".\n"
   "You then need to give me at least one %s and %s revision.\n"
   "You can use \"git bisect %s\" and "\"git bisect %s\" for that.")

Stephan

Re: [PATCH] config: added --expiry-date type support

[hsed]

On 2017-11-12 14:55, Jeff King wrote:

Hi, and welcome to the list. Thanks for working on this (for those of
you on the list, this was one of the tasks at the hackathon this
weekend).


It was a pleasure meeting everyone and a great experience!



Kevin already mentioned a few things about the commit message, which I
agree with.


Sorry about that and the commit message formatting,
now that my mail is being received by git@vger I will try sending 
patches

with the required text, etc.



It's great that there are new tests. We'll probably need some
documentation, too (especially users will need to know what the output
format means).



True, looking at the repo I found a document here[0]
Should I try editing this to add the new option?


@@ -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")),


We seem to use both "expire" and "expiry" throughout the code and in
user-facing bits (e.g., "gc.reflogExpire" and "gc.logExpiry"). I don't
have a real preference for one versus the other. I just mention it 
since

whatever we choose here will be locked in to the interface forever.



I am not sure why do we need to use the 'expir(e/y)' keyword?
I think the parse_expiry_date() function still worked for past dates
is that intended?

Would having it as just '--date' suffice or do you plan to
have --date-type which will be different from expiry dates?

Anyways, I will use whatever keyword you think is more suitable. Please 
let me know.


@@ -159,6 +161,12 @@ 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 = malloc(sizeof(*t));
+   if(git_config_expiry_date(, key_, value_) < 0)
+   return -1;
+   strbuf_addf(buf, "%"PRItime, *t);
+   free((timestamp_t *)t);
} else if (value_) {


Since we only need the timestamp variable within this block, we don't
need to use a pointer. We can just do something like:

  } else if (types == TYPE_EXPIRY_DATE) {
timestamp_t t;
if (git_config_expiry_date(, key_, value_) < 0)
return -1;
strbuf_addf(buf, "%"PRItime", t);
  }

Note that your new git_config_expiry_date would want to take just a
regular pointer, rather than a pointer-to-pointer. I suspect you picked
that up from git_config_pathname(). It needs the double pointer because
it's storing a string (which is itself a pointer), but we don't need
that here.


Yes, I got it from the pathname function, I'll change this to just 
pointer.





diff --git a/config.c b/config.c
index 903abf9533b18..caa2fd5fb6915 100644
--- a/config.c
+++ b/config.c
@@ -990,6 +990,15 @@ 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))
+   die(_("failed to parse date_string in: '%s'"), value);
+   return 0;
+}


I was surprised that we don't already have a function that does this,
since we parse expiry config elsewhere. We do, but it's just local to
builtin/reflog.c. So perhaps as a preparatory step we should add this
function and convert reflog.c to use it, dropping its custom
parse_expire_cfg_value().


Ok, I will make these changes in reflog.c.



What's the purpose of the "!!" before parse_expiry_date()? The usual
idiom for that to normalize a non-zero value into "1", but we don't 
care

here. I think just:

  if (parse_expiry_date(value, timestamp))
die(...);

would be sufficient.


No real purpose, I saw it in prev code but I guess that had a different
purpose (as you mentioned) I'll change that.


diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 364a537000bbb..59a35be89e511 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -901,6 +901,31 @@ test_expect_success 'get --path barfs on boolean 
variable' '

test_must_fail git config --get --path path.bool
 '

+test_expect_success 'get --expiry-date' '
+   cat >.git/config <<-\EOF &&
+   [date]
+   valid1 = "Fri Jun 4 15:46:55 2010"
+   valid2 = "2017/11/11 11:11:11PM"

Re: [PATCH v16 Part II 5/8] bisect--helper: `bisect_next_check` shell function in C

[Stephan Beyer]

> @@ -21,6 +22,7 @@ static const char * const git_bisect_helper_usage[] = {
>   N_("git bisect--helper --bisect-reset []"),
>   N_("git bisect--helper --bisect-write
>  []"),
>   N_("git bisect--helper --bisect-check-and-set-terms  
>  "),
> + N_("git bisect--helper --bisect-next-check []  
> "),

I think the order is wrong. It is   []

Re: [PATCH v16 Part II 5/8] bisect--helper: `bisect_next_check` shell function in C

[Stephan Beyer]

Hi,

another minor:

On 10/27/2017 05:06 PM, Pranit Bauva wrote:
> @@ -264,6 +271,79 @@ static int check_and_set_terms(struct bisect_terms 
> *terms, const char *cmd)
>   return 0;
>  }
>  
> +static int mark_good(const char *refname, const struct object_id *oid,
> +  int flag, void *cb_data)
> +{
> + int *m_good = (int *)cb_data;
> + *m_good = 0;
> + return 1;
> +}
> +
> +static int bisect_next_check(const struct bisect_terms *terms,
> +  const char *current_term)
> +{
> + int missing_good = 1, missing_bad = 1, retval = 0;
> + const char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
> + const char *good_glob = xstrfmt("%s-*", terms->term_good);
> +
> + if (ref_exists(bad_ref))
> + missing_bad = 0;
> +
> + for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/",
> +  (void *) _good);
> +
> + if (!missing_good && !missing_bad)
> + goto finish;
> +
> + if (!current_term)
> + goto fail;
> +
> + if (missing_good && !missing_bad && current_term &&

This check for "current_term" is not necessary; it can be asserted to be
non-NULL, otherwise you would have jumped to "fail"

Stephan

Re: should "git bisect" support "git bisect next?"

[Theodore Ts'o]

On Sun, Nov 12, 2017 at 03:21:57PM +0100, Christian Couder wrote:
> 
> Yeah I agree that it might be something interesting for the user to do.
> But in this case the sequence in which you give the good and the bad
> commits is not important.
> Only the last bad commit and the set of good commits that were given
> are important.

Is it really true that of the bad commits, only the last one is significant?

Suppose we have a git tree that looks like this:

  *---*---*---*---*---*---M2---*---B1
  ||
  G1--*--D1---*---*---*---B2-\ |
  |   \/
  *---*---*---B3--*---M1--/

If we know that commits B2 and B3 are bad, if we assume that all
commits before the "bad" commit are good, all commits after the "bad"
commit are bad, can we not deduce that commit D1 should also be "bad"?

  - Ted

Re: [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming

[Kevin Daudt]

On Thu, Nov 02, 2017 at 12:24:07PM +0530, Kaartic Sivaraam wrote:
> From: Kaartic Sivaraam 
> 
> When trying to rename an inexistent branch to with a name of a branch

This sentence does not read well. Probably s/with a/the/ helps.

> that already exists the rename failed specifying the new branch name
> exists rather than specifying that the branch trying to be renamed
> doesn't exist.
> 
> [..]
> 
> Note: Thanks to the strbuf API that made it possible to easily construct
> the composite error message strings!

I'm not sure this note adds a lot, since the strbuf API is not that new.

Kevin

Re: [PATCH v2] gpg-interface: strip CR chars for Windows gpg2

[Eric Sunshine]

Thanks for the re-roll...

On Sun, Nov 12, 2017 at 8:07 AM, Jerzy Kozera  wrote:
> This fixes the issue with newlines being \r\n and not being displayed
> correctly when using gpg2 for Windows, as reported at
> https://github.com/git-for-windows/git/issues/1249

It's still not clear from this description what "not being displayed
correctly" means. Ideally, the commit message should stand on its own,
explaining exactly what problem the patch is solving, without the
reader having to chase URLs to pages (which might disappear). If you
could summarize the problem and solution in your own words in such a
way that your description itself conveys enough information for
someone not familiar with that problem report to understand the
problem, then that would likely make a good commit message.

More below...

> Issues with non-ASCII characters remain for further investigation.
>
> Signed-off-by: Jerzy Kozera 
> ---
> diff --git a/gpg-interface.c b/gpg-interface.c
> @@ -145,6 +145,20 @@ const char *get_signing_key(void)
> +/* Strip CR from the CRLF line endings, in case we are on Windows. */
> +static void strip_cr(struct strbuf *buffer, size_t bottom) {

It's not at all clear what 'bottom' means. In the original, when the
code was inline, the surrounding context would likely have given a
good clue to the meaning of 'bottom', but here stand-alone, it conveys
little or nothing. Perhaps a better name for this argument would be
'start_at' or 'from' or something.

> +   size_t i, j;
> +   for (i = j = bottom; i < buffer->len; i++)
> +   if (!(i < buffer->len - 1 &&
> +   buffer->buf[i] == '\r' &&
> +   buffer->buf[i + 1] == '\n')) {

Hmm, was this tested? If I'm reading this correctly, this strips out
the entire CRLF pair, whereas the original code only stripped the CR
and left what followed it (typically LF) alone. Junio's suggestion was
to enhance this to be more careful and strip CR only when followed
immediately by LF (but to leave the LF intact). Therefore, this seems
like a regression.

> +   if (i != j)
> +   buffer->buf[j] = buffer->buf[i];
> +   j++;
> +   }
> +   strbuf_setlen(buffer, j);
> +}
> +
>  /*
>   * Create a detached signature for the contents of "buffer" and append
>   * it after "signature"; "buffer" and "signature" can be the same
> @@ -155,7 +169,7 @@ int sign_buffer(struct strbuf *buffer, struct strbuf 
> *signature, const char *sig
>  {
> struct child_process gpg = CHILD_PROCESS_INIT;
> int ret;
> -   size_t i, j, bottom;
> +   size_t bottom;
> struct strbuf gpg_status = STRBUF_INIT;
>
> argv_array_pushl(,
> @@ -180,14 +194,7 @@ int sign_buffer(struct strbuf *buffer, struct strbuf 
> *signature, const char *sig
> if (ret)
> return error(_("gpg failed to sign the data"));
>
> -   /* Strip CR from the line endings, in case we are on Windows. */
> -   for (i = j = bottom; i < signature->len; i++)
> -   if (signature->buf[i] != '\r') {
> -   if (i != j)
> -   signature->buf[j] = signature->buf[i];
> -   j++;
> -   }
> -   strbuf_setlen(signature, j);
> +   strip_cr(signature, bottom);
>
> return 0;
>  }
> @@ -230,6 +237,8 @@ int verify_signed_buffer(const char *payload, size_t 
> payload_size,
> sigchain_push(SIGPIPE, SIG_IGN);
> ret = pipe_command(, payload, payload_size,
>gpg_status, 0, gpg_output, 0);
> +   strip_cr(gpg_status, 0);
> +   strip_cr(gpg_output, 0);
> sigchain_pop(SIGPIPE);
>
> delete_tempfile();
> --
> 2.14.2.windows.3

Re: NO_MSGFMT

[Christian Couder]

On Sun, Nov 12, 2017 at 12:57 PM, Jeff King  wrote:
> On Sun, Aug 13, 2017 at 12:58:13AM -0400, Jeff King wrote:
>
>> On Sat, Aug 12, 2017 at 03:44:17PM +0200, Dominik Mahrer (Teddy) wrote:
>>
>> > Hi all
>> >
>> > I'm compiling git from source code on a mashine without msgfmt. This leads
>> > to compile errors. To be able to compile git I created a patch that at 
>> > least
>> > works for me:
>>
>> Try:
>>
>>   make NO_MSGFMT=Nope NO_GETTEXT=Nope
>>
>> This also works:
>>
>>   make NO_GETTEXT=Nope NO_TCLTK=Nope
>>
>> The flags to avoid gettext/msgfmt are sadly different between git itself
>> and git-gui/gitk, which we include as a subproject. It would be a useful
>> patch to harmonize though (probably by accepting both in all places for
>> compatibility).
>
> I saw somebody else today run into problems about gettext, so I thought
> I'd revisit this and write that patch. It turns out the situation is
> slightly different than I thought. So no patch, but I wanted to report
> here what I found.
>
> It's true that the option is called NO_GETTEXT in git.git, but NO_MSGFMT
> in the tcl programs we pull in. So I figured to start with a patch that
> turns on NO_MSGFMT automatically when NO_GETTEXT is set. But it's
> not necessary.
>
> The gitk and git-gui tests actually check that msgfmt is available.
> If it isn't, they automatically fall back to using a pure-tcl
> implementation. So there's generally no need to set NO_MSGFMT at
> all.
>
> But that fallback is implemented using tcl. So if you _also_ don't have
> tcl installed (and I don't), you get quite a confusing output from make:
>
>   $ make -j1
> SUBDIR git-gui
> MSGFMT po/pt_pt.msg Makefile:252: recipe for target 'po/pt_pt.msg' failed
> make[1]: *** [po/pt_pt.msg] Error 127
>
> If you run with V=1, you can see that it's not running msgfmt at all,
> but:
>
>   tclsh po/po2msg.sh --statistics --tcl -l pt_pt -d po/ po/pt_pt.po
>
> So my takeaways are:
>
>   1. You should never need to set NO_MSGFMT; it falls back
>  automatically.
>
>   2. If you don't have gettext, you should set NO_GETTEXT to tell the
>  rest of git not to use it.
>
>   3. If you see msgfmt errors even after NO_GETTEXT, try NO_TCLTK.

I wonder if something like following patch could help, as anyway the
build will fail if Tcl/Tk is not installed and people often prefer
builds failing at the beginning rather than towards the end.

diff --git a/Makefile b/Makefile
index ee9d5eb11e..9789027739 100644
--- a/Makefile
+++ b/Makefile
@@ -1636,6 +1636,13 @@ ifeq ($(TCLTK_PATH),)
 NO_TCLTK = NoThanks
 endif

+ifndef NO_TCLTK
+   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 it")
+   endif
+endif
+
 ifeq ($(PERL_PATH),)
 NO_PERL = NoThanks
 endif

Re: [PATCH 2/2] stash: implement builtin stash helper

[Joel Teichroeb]

Thanks for your comments! I've incorporated them all for the next patch set.

[PATCH] Fix NO_LIBPCRE1_JIT to fully disable JIT

[Charles Bailey]

From: Charles Bailey 

If you have a pcre1 library which is compiled with JIT enabled then
PCRE_STUDY_JIT_COMPILE will be defined whether or not the
NO_LIBPCRE1_JIT configuration is set.

This means that we enable JIT functionality when calling pcre_study
even if NO_LIBPCRE1_JIT has been explicitly set and we just use plain
pcre_exec later.

Fix this by using own macro (GIT_PCRE_STUDY_JIT_COMPILE) which we set to
PCRE_STUDY_JIT_COMPILE only if NO_LIBPCRE1_JIT is not set and define to
0 otherwise, as before.
---

I was bisecting an issue with the PCRE support that was causing a test
suite failure on our Solaris builds and reached fbaceaac47 ("grep: add
support for the PCRE v1 JIT API"). It appeared to be a misaligned memory
access somewhere inside the libpcre code. I tried disabling the use of
JIT with NO_LIBPCRE1_JIT but it turned out that even with this set we
were still triggering the JIT code path in the call to pcre_study.

Yes, we probably should fix our PCRE1 library build on Solaris or move
to PCRE2, but really NO_LIBPCRE1_JIT should have prevented us from
triggering this crash.

 grep.c | 2 +-
 grep.h | 5 +++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/grep.c b/grep.c
index ce6a48e..d0b9b6c 100644
--- a/grep.c
+++ b/grep.c
@@ -387,7 +387,7 @@ static void compile_pcre1_regexp(struct grep_pat *p, const 
struct grep_opt *opt)
if (!p->pcre1_regexp)
compile_regexp_failed(p, error);
 
-   p->pcre1_extra_info = pcre_study(p->pcre1_regexp, 
PCRE_STUDY_JIT_COMPILE, );
+   p->pcre1_extra_info = pcre_study(p->pcre1_regexp, 
GIT_PCRE_STUDY_JIT_COMPILE, );
if (!p->pcre1_extra_info && error)
die("%s", error);
 
diff --git a/grep.h b/grep.h
index 52aecfa..399381c 100644
--- a/grep.h
+++ b/grep.h
@@ -7,11 +7,12 @@
 #if PCRE_MAJOR >= 8 && PCRE_MINOR >= 32
 #ifndef NO_LIBPCRE1_JIT
 #define GIT_PCRE1_USE_JIT
+#define GIT_PCRE_STUDY_JIT_COMPILE PCRE_STUDY_JIT_COMPILE
 #endif
 #endif
 #endif
-#ifndef PCRE_STUDY_JIT_COMPILE
-#define PCRE_STUDY_JIT_COMPILE 0
+#ifndef GIT_PCRE_STUDY_JIT_COMPILE
+#define GIT_PCRE_STUDY_JIT_COMPILE 0
 #endif
 #if PCRE_MAJOR <= 8 && PCRE_MINOR < 20
 typedef int pcre_jit_stack;
-- 
2.10.2

TO CLAIM YOUR FUNDS IS FREE OF CHARGE

[QATAR FOUNDATION]

Attention:Sir/Madam

THIS OFFICIAL UN GUARANTEE LETTER COVER,

You have been selected to receive (950,000.00 EURO) as charity donations / aid 
from the Qatar Foundation. Please kindly provide the
below information details mention.

Therefore contact our Agent Mrs.Linda Hall
Inspector Admin -(QF) Payment Center.
contact E-mail; ( qatarfoundation...@gmail.com )


Meanwhile send her your full delivery details:
(1)Your Full Name=
(2)Mobile Phone Number==
(3)Current Home Address 
(4)Fax Number
(5)Country
(6)City==
(7)Postal Address ==


Note this compensation including a Victims and international businesses that 
failed due to Government problems from Europe country.
This donation funds  will be send to you Via ATM Master Card

 Yours sincerely,
 Engineer Saad Al Muhannadi.
 President of the Qatar Foundation.

Re: [PATCH] mru: Replace mru.[ch] with list.h implementation

[Jeff King]

On Sun, Nov 12, 2017 at 12:49:17PM +, Gargi Sharma wrote:

> > Sort of a side note, but seeing these two list pointers together makes
> > me wonder what we should do with the list created by the "next" pointer.
> > It seems like there are three options:
> >
> >   1. Convert it to "struct list_head", too, for consistency.
> >
> >   2. Leave it as-is. We never delete from the list nor do any fancy
> >  manipulation, so it doesn't benefit from the reusable code.
> >
> >   3. I wonder if we could drop it entirely, and just keep a single list
> >  of packs, ordered by mru. I'm not sure if anybody actually cares
> >  about accessing them in the "original" order. That order is
> >  reverse-chronological (by prepare_packed_git()), but I think that
> >  was mostly out of a sense that recent packs would be accessed more
> >  than older ones (but having a real mru strategy replaces that
> >  anyway).
> >
> > What do you think?
> I think in the long run, it'll be easier if there is only a single
> list of packs given
> that no one needs to access the list in order.

Yeah, it's that "given..." that makes me just a little nervous that I'm
missing something.

> If we go down road 1/3, would it be better if I sent an entirely
> different patch or
> a patch series with patch 1 as removing mru[.ch] and patch 2 as removing
> next pointer from the struct?

I think you could do it as a 2-patch series like that, or you could send
the first patch now (since I think it stands on its own merits) and do
the other one later on top.

> > This matches the original code, which did the clear/re-create, resetting
> > the mru to the "original" pack order. But I do wonder if that's actually
> > necessary. Could we skip that and just add any new packs to the list?
> 
> But if we do not clear the older entries from the list, wouldn't there be a
> problem when you access packed_git_mru->next, since that will be populated
> instead of being empty? Or am I misunderstanding something here?

What I mean is that instead of clearing and re-adding all of the packs
(including any new ones we picked up by rescanning the directory), we
would _just_ add new ones to the list.

So I think we'd scrap this whole "set up the mru" preparation here and
just teach install_packed_git() to add the new pack to the MRU list.

-Peff

Re: [PATCH] Make t4201-shortlog.sh test more robust

[Jeff King]

On Sun, Nov 12, 2017 at 03:25:23PM +, Charles Bailey wrote:

> From: Charles Bailey 
> 
> The test for '--abbrev' in t4201-shortlog.sh assumes that the commits
> generated in the test can always be uniquely abbreviated to 5 hex digits
> but this is not always the case. If you were unlucky and happened to run
> the test at (say) Thu Jun 22 03:04:49 2017 +, you would find that
> the first commit generated would collide with a tree object created
> later in the same test.
> 
> This can be simulated in the version of t4201-shortlog.sh prior to this
> commit by setting GIT_COMMITTER_DATE and GIT_AUTHOR_DATE to 1498100689
> after sourcing test-lib.sh.
> 
> Change the test to test --abbrev=35 instead of --abbrev=5 to almost
> completely avoid the possibility of a partial collision and add a call
> to test_tick in the setup to make the test repeatable.

This generally makes sense to me, but I think there's one thing unsaid
in this last paragraph: why is it OK to switch from 5 to 35?

The answer is that the test is just checking that --abbrev is respected,
and doesn't care whether it's making things larger or smaller. There are
other cases of --abbrev=5 in the test suite which might fall into the
same boat.

You're also really doing two things to fix the problem here, either one
of which would have been sufficient: increasing the abbreviation size
and using test_tick to get a deterministic run.

I'm OK with doing that here, but some of the other --abbrev=5 cases
might already be fine due to using test_tick appropriately.

> diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
> index 9df054b..da10478 100755
> --- a/t/t4201-shortlog.sh
> +++ b/t/t4201-shortlog.sh
> @@ -9,6 +9,7 @@ test_description='git shortlog
>  . ./test-lib.sh
>  
>  test_expect_success 'setup' '
> + test_tick &&

Charles and I had a discussion off-list whether it's appropriate to
test_tick once, but not for each commit (simply because it's a minor
pain to remember to do so before each commit).

Doing it once is enough to make the test deterministic, and for this
particular case we don't actually care at all whether all of the commits
have the exact same timestamp. So I think it's fine.

One option is to try replacing the ad-hoc commits with test_commit, but
it turns out to be quite ugly in this case, as the tests depend on a lot
of oddly-formatted commit messages.

-Peff

Re: [add-default-config 5/5] fix return code on default + add tests

[Jeff King]

On Sun, Nov 12, 2017 at 03:00:40PM +, Soukaina NAIT HMID wrote:

> diff --git a/builtin/config.c b/builtin/config.c
> index eab81c5627091..29c5f55f27a57 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -261,9 +261,12 @@ static int get_value(const char *key_, const char 
> *regex_)
>  
>   if (values.nr == 0 && default_value) {
>   if(types == TYPE_INT || types == TYPE_BOOL || types == 
> TYPE_BOOL_OR_INT || types == TYPE_PATH ) {
> - char* xstr = normalize_value(key, default_value);
> - fwrite(xstr, 1, strlen(xstr), stdout);
> - fwrite("\n", 1, 1, stdout);
> + if(strlen(default_value)) {
> + char* xstr = normalize_value(key, 
> default_value);
> + fwrite(xstr, 1, strlen(xstr), stdout);
> + fwrite("\n", 1, 1, stdout);
> + ret = 0;
> + }
>   }

OK, fixing up the return value is a good thing (though again, I think if
we place our default_value in the list earlier that will just fall out
naturally).

I'm not sure why we care if the default_value string is empty or not. It
should be allowed to default to an empty string, I'd think.

> diff --git a/t/t9904-default.sh b/t/t9904-default.sh
> new file mode 100755
> index 0..8e838f512298b
> --- /dev/null
> +++ b/t/t9904-default.sh

We usually try to group tests with similar-numbered ones. Most of the
config tests are in the t13xx area. Probably "t1310-config-default.sh"
would be the right place (or if there really are just a few tests, which
I think may be all we need, they can just go into t1300).

> +boolean()
> +{
> + slot=$([ "$#" == 3 ] && echo $3 || echo "no.such.slot") &&
> + actual=$(git config --default "$1" --bool "$slot") &&
> + test "$actual" = "$2"
> +}

A minor style nit, but we usually prefer "test" instead of "[" for
conditionals. It took me a while to figure out how this function was
meant to be used. It might be worth adding a comment. Though most of it
was due to the first line, which I think can just be written as:

  slot=${3:-no.such.slot}

(or you could even just write that directly in the second line).

That's a bit more idiomatic for our shell scripts.

> +test_expect_success 'empty value for boolean' '
> + invalid_boolean ""
> +'

There are a lot of tests here about type interpretation, but I think
that should be largely orthogonal to the --default feature. Once it's
written in a way that's independent of the type, I think we can assume
that if "--default" works for one type, it should work with others
without being exhaustive.

So I think what we really want to test from this series is:

  1. --default kicks in when no matching config is found

  2. --default does not kick in when config _is_ found

  3. (optional) we complain about --default with non-get actions

  4. --color works as a type for "get" operations

  5. --color is not normalized for "set" operations; if you do:

   git config --color some.key red

 we should write "red" into the config file, not the ANSI codes.

I know the reason you were looking into t4026 originally because it was
the only spot that used --get-color in the whole test suite. But its use
of "--get-color" is largely orthogonal to what it's testing. It cares
about parsing the specific colors, but just didn't have another easy way
to convince Git to parse a bunch of colors without having to pick the
results out of diff or log output.

I'd be OK with converting that to use "--color --default" instead of
--get-color, but if we do so we should make sure that there's some
coverage of "--get-color" elsewhere in the config tests (not checking
every possible color variation, but just making sure that it can
actually look up any color with it).

-Peff

Re: [add-default-config 4/5] add defaults for path/int/bool

[Jeff King]

On Sun, Nov 12, 2017 at 03:00:40PM +, Soukaina NAIT HMID wrote:

> @@ -256,8 +258,15 @@ static int get_value(const char *key_, const char 
> *regex_)
>   fwrite(buf->buf, 1, buf->len, stdout);
>   strbuf_release(buf);
>   }
> - free(values.items);
>  
> + if (values.nr == 0 && default_value) {
> + if(types == TYPE_INT || types == TYPE_BOOL || types == 
> TYPE_BOOL_OR_INT || types == TYPE_PATH ) {
> + char* xstr = normalize_value(key, default_value);
> + fwrite(xstr, 1, strlen(xstr), stdout);
> + fwrite("\n", 1, 1, stdout);
> + }
> + }
> + free(values.items);

OK, this location makes more sense to me for handling --default. I think
it's still a bit lower in the function than I'd expect, though.

The loop just above the code you added is showing all of the values we
found. So I think that's the spot where'd say "we didn't find any
values; pretend like we found default_value". Including, I think, making
sure that the return value from the function is still 0. So realy right
after config_with_options() returns, I'd expect to check something like:

  if (!values.nr && default_value) {
  /* insert default_value into values list */
  }

We'd also want to use format_config(), not normalize_config(). We do
format_config() to show values, whereas normalize_config() is usually
for values we're putting into a config file (so for example TYPE_PATH
doesn't get normalized, but we would want it converted here to show the
user).

I'm also not sure that we need to check "types" as you have here.
Wouldn't we want to apply the default regardless of type, and let
format_config() handle it?

> @@ -272,6 +281,7 @@ static int get_value(const char *key_, const char *regex_)
>   return ret;
>  }
>  
> +
>  static char *normalize_value(const char *key, const char *value)

Watch out for stray changes like this one creeping into your commits.

> diff --git a/t/t4026-color2.sh b/t/t4026-color2.sh
> deleted file mode 100755
> index 695ce9dd6f8d4..0

This part is obviously good and rectifying the problem from patch 3.
Once they're squashed together, we won't see it at all. :)

-Peff

Re: [add-default-config 3/5] add same test for new command format with --default and --color

[Jeff King]

On Sun, Nov 12, 2017 at 03:00:40PM +, Soukaina NAIT HMID wrote:

> ---
>  t/t4026-color2.sh | 129 
> ++
>  1 file changed, 129 insertions(+)
>  create mode 100755 t/t4026-color2.sh

I'll skip reviewing this one, since the file goes away later in the
series.

-Peff

Re: [add-default-config 2/5] adding default to color

[Jeff King]

On Sun, Nov 12, 2017 at 03:00:40PM +, Soukaina NAIT HMID wrote:

> diff --git a/builtin/config.c b/builtin/config.c
> index 124a682d50fa8..9df2d9c43bcad 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -30,6 +30,7 @@ static int end_null;
>  static int respect_includes_opt = -1;
>  static struct config_options config_options;
>  static int show_origin;
> +static const char *default_value;
> [...]
> + OPT_STRING(0, "default", _value, N_("default-value"), N_("sets 
> default for bool/int/path/color when no value is returned from config")),

These hunks make sense. We're adding a new "--default" option that would
kick in when you try to look up a key and it isn't present.

I think we can skip the "bool/int/path/color" thing in the help string.
We would want this to kick in for every type, right?  The only
constraint is that we are doing a "get" operation. It wouldn't make any
sense to use "--default" when setting a variable, listing, etc. Should
we catch these cases and return an error?

We'd also want to mention this in Documentation/git-config.txt.

> @@ -47,6 +48,7 @@ static int show_origin;
>  #define ACTION_GET_COLOR (1<<13)
>  #define ACTION_GET_COLORBOOL (1<<14)
>  #define ACTION_GET_URLMATCH (1<<15)
> +#define ACTION_GET_COLORORDEFAULT (1<<16)

I'm not sure I understand this part, though. Providing a default should
be something that goes along with a "get" action, but isn't its own
action.

> +static void get_color_default(const char *var)
> +{
> + get_color(var, default_value);
> +}
> +

And here we're just applying --default to colors, but we'd eventually
want them for everything. I think that's fixed later in the series, so
I'll keep reading. But I'd expect a function like get_value() to be
detecting the case where we got no hits and filling in the default_value
there, as if we had read it from the config file.

-Peff

[PATCH] Make t4201-shortlog.sh test more robust

[Charles Bailey]

From: Charles Bailey 

The test for '--abbrev' in t4201-shortlog.sh assumes that the commits
generated in the test can always be uniquely abbreviated to 5 hex digits
but this is not always the case. If you were unlucky and happened to run
the test at (say) Thu Jun 22 03:04:49 2017 +, you would find that
the first commit generated would collide with a tree object created
later in the same test.

This can be simulated in the version of t4201-shortlog.sh prior to this
commit by setting GIT_COMMITTER_DATE and GIT_AUTHOR_DATE to 1498100689
after sourcing test-lib.sh.

Change the test to test --abbrev=35 instead of --abbrev=5 to almost
completely avoid the possibility of a partial collision and add a call
to test_tick in the setup to make the test repeatable.

Signed-off-by: Charles Bailey 
---
 t/t4201-shortlog.sh | 5 +++--
 t/test-lib.sh   | 7 ---
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index 9df054b..da10478 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -9,6 +9,7 @@ test_description='git shortlog
 . ./test-lib.sh
 
 test_expect_success 'setup' '
+   test_tick &&
echo 1 >a1 &&
git add a1 &&
tree=$(git write-tree) &&
@@ -59,7 +60,7 @@ fuzz() {
file=$1 &&
sed "
s/$_x40/OBJECT_NAME/g
-   s/$_x05/OBJID/g
+   s/$_x35/OBJID/g
s/^ \{6\}[CTa].*/  SUBJECT/g
s/^ \{8\}[^ ].*/CONTINUATION/g
" <"$file" >"$file.fuzzy" &&
@@ -81,7 +82,7 @@ test_expect_success 'pretty format' '
 
 test_expect_success '--abbrev' '
sed s/SUBJECT/OBJID/ expect.template >expect &&
-   git shortlog --format="%h" --abbrev=5 HEAD >log &&
+   git shortlog --format="%h" --abbrev=35 HEAD >log &&
fuzz log >log.predictable &&
test_cmp expect log.predictable
 '
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 9b61f16..116bd6a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -175,9 +175,10 @@ esac
 
 # Convenience
 #
-# A regexp to match 5 and 40 hexdigits
+# A regexp to match 5, 35 and 40 hexdigits
 _x05='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
-_x40="$_x05$_x05$_x05$_x05$_x05$_x05$_x05$_x05"
+_x35="$_x05$_x05$_x05$_x05$_x05$_x05$_x05"
+_x40="$_x35$_x05"
 
 # Zero SHA-1
 _z40=
@@ -193,7 +194,7 @@ LF='
 # when case-folding filenames
 u200c=$(printf '\342\200\214')
 
-export _x05 _x40 _z40 LF u200c EMPTY_TREE EMPTY_BLOB
+export _x05 _x35 _x40 _z40 LF u200c EMPTY_TREE EMPTY_BLOB
 
 # Each test should start with something like this, after copyright notices:
 #
-- 
2.10.2

Re: [PATCH 2/2] stash: implement builtin stash helper

[Thomas Gummerer]

On 11/10, Joel Teichroeb wrote:
> Start moving stash functions over to builtin c code and call
> them in the shell script, instead of converting it all at
> once.
> 
> Signed-off-by: Joel Teichroeb 
> ---

Thanks for working on this!  I like the approach of converting this
one command at a time.  I think it would be even better if this was a
series of patches, converting stash one command at a time, instead of
adding multiple multiple commands to stash helper in one patch.  This
would make reviewing the patches a bit easier.

For example this could look like:
[2/5] stash: convert apply to builtin
[3/5] stash: convert branch to builtin
[4/5] stash: convert drop to builtin
[5/5] stash: convert pop to builtin

Some comments from me below.

>  Makefile|   1 +
>  builtin.h   |   1 +
>  builtin/stash--helper.c | 516 
> 
>  git-stash.sh| 134 +
>  git.c   |   1 +
>  5 files changed, 526 insertions(+), 127 deletions(-)
>  create mode 100644 builtin/stash--helper.c
> 
> diff --git a/Makefile b/Makefile
> index ee9d5eb11..3a9bd4d57 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1000,6 +1000,7 @@ BUILTIN_OBJS += builtin/send-pack.o
>  BUILTIN_OBJS += builtin/shortlog.o
>  BUILTIN_OBJS += builtin/show-branch.o
>  BUILTIN_OBJS += builtin/show-ref.o
> +BUILTIN_OBJS += builtin/stash--helper.o
>  BUILTIN_OBJS += builtin/stripspace.o
>  BUILTIN_OBJS += builtin/submodule--helper.o
>  BUILTIN_OBJS += builtin/symbolic-ref.o
> diff --git a/builtin.h b/builtin.h
> index 42378f3aa..a14fd85b0 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -219,6 +219,7 @@ extern int cmd_shortlog(int argc, const char **argv, 
> const char *prefix);
>  extern int cmd_show(int argc, const char **argv, const char *prefix);
>  extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
>  extern int cmd_status(int argc, const char **argv, const char *prefix);
> +extern int cmd_stash__helper(int argc, const char **argv, const char 
> *prefix);
>  extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
>  extern int cmd_submodule__helper(int argc, const char **argv, const char 
> *prefix);
>  extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> new file mode 100644
> index 0..c8cb667fe
> --- /dev/null
> +++ b/builtin/stash--helper.c
> @@ -0,0 +1,516 @@
> +#include "builtin.h"
> +#include "config.h"
> +#include "parse-options.h"
> +#include "refs.h"
> +#include "lockfile.h"
> +#include "cache-tree.h"
> +#include "unpack-trees.h"
> +#include "merge-recursive.h"
> +#include "argv-array.h"
> +#include "run-command.h"
> +
> +static const char * const git_stash_usage[] = {

s/stash/&_helper/ maybe?  This is how we call it in the other helpers
as well.

> + N_("git stash--helper drop [-q|--quiet] []"),
> + N_("git stash--helper pop [--index] [-q|--quiet] []"),
> + N_("git stash--helper apply [--index] [-q|--quiet] []"),
> + N_("git stash--helper branch  []"),
> + N_("git stash--helper clear"),
> + NULL
> +};
> +
> +static const char * const git_stash_drop_usage[] = {
> + N_("git stash--helper drop [-q|--quiet] []"),
> + NULL
> +};
> +
> +static const char * const git_stash_pop_usage[] = {
> + N_("git stash--helper pop [--index] [-q|--quiet] []"),
> + NULL
> +};
> +
> +static const char * const git_stash_apply_usage[] = {
> + N_("git stash--helper apply [--index] [-q|--quiet] []"),
> + NULL
> +};
> +
> +static const char * const git_stash_branch_usage[] = {
> + N_("git stash--helper branch  []"),
> + NULL
> +};
> +
> +static const char * const git_stash_clear_usage[] = {
> + N_("git stash--helper clear"),
> + NULL
> +};
> +
> +static const char *ref_stash = "refs/stash";
> +static int quiet;
> +static char stash_index_path[PATH_MAX];
> +
> +struct stash_info {
> + struct object_id w_commit;
> + struct object_id b_commit;
> + struct object_id i_commit;
> + struct object_id u_commit;
> + struct object_id w_tree;
> + struct object_id b_tree;
> + struct object_id i_tree;
> + struct object_id u_tree;
> + const char *message;
> + const char *revision;
> + int is_stash_ref;
> + int has_u;
> + const char *patch;
> +};
> +
> +static int get_stash_info(struct stash_info *info, const char *commit)
> +{
> + struct strbuf w_commit_rev = STRBUF_INIT;
> + struct strbuf b_commit_rev = STRBUF_INIT;
> + struct strbuf w_tree_rev = STRBUF_INIT;
> + struct strbuf b_tree_rev = STRBUF_INIT;
> + struct strbuf i_tree_rev = STRBUF_INIT;
> + struct strbuf u_tree_rev = STRBUF_INIT;
> + struct strbuf commit_buf = STRBUF_INIT;
> + struct strbuf symbolic = STRBUF_INIT;
> + struct strbuf out = STRBUF_INIT;
> + int ret;
> + const char *revision = commit;
> + char 

Re: [add-default-config 1/5] add --color option to git config

[Jeff King]

On Sun, Nov 12, 2017 at 03:00:40PM +, Soukaina NAIT HMID wrote:

> From: Soukaina NAIT HMID 
> 
> Signed-off-by: Soukaina NAIT HMID 

Hi Soukaina, and welcome to the list! Thanks for working on these
patches.

I'll go through and make inline comments on your patches, but first a
few overall issues:

 - you have five patches in your series, some of which are backing out
   changes made by the other patches. It's normal in the Git community
   to use "git rebase -i" to squash down your changes to "clean" patches
   that form a sequence. For your topic, I'd expect to see two patches
   in the end:

 1. Add a "config --default" option (and documentation and tests).

 2. Add a "config --color" type (and documentation and tests).

 - your commit messages are mostly empty. :) This is a good place to
   describe the motivation for the patch, document any design decisions,
   discuss any alternatives, etc. This is helpful for reviewers to
   understand what you're trying to achieve, and for people later who
   discover your commit from "git log".

> diff --git a/builtin/config.c b/builtin/config.c
> index d13daeeb55927..124a682d50fa8 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -80,6 +80,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, "color", , N_("find the color configured"), 
> ACTION_GET_COLOR),

I think we'd want this "--color" to actually be a type, not a separate
action. I.e., so that it behaves --int, --bool, etc. Part of the goal
(well, my goal) was to make accessing color config more like other types
of config.

-Peff

[add-default-config 3/5] add same test for new command format with --default and --color

[Soukaina NAIT HMID]

From: Soukaina NAIT HMID 

Signed-off-by: Soukaina NAIT HMID 
---
 t/t4026-color2.sh | 129 ++
 1 file changed, 129 insertions(+)
 create mode 100755 t/t4026-color2.sh

diff --git a/t/t4026-color2.sh b/t/t4026-color2.sh
new file mode 100755
index 0..695ce9dd6f8d4
--- /dev/null
+++ b/t/t4026-color2.sh
@@ -0,0 +1,129 @@
+#!/bin/sh
+#
+# Copyright (c) 2008 Timo Hirvonen
+#
+
+test_description='Test diff/status color escape codes'
+. ./test-lib.sh
+
+ESC=$(printf '\033')
+color()
+{
+   actual=$(git config --default "$1" --color no.such.slot) &&
+   test "$actual" = "${2:+$ESC}$2"
+}
+
+invalid_color()
+{
+   test_must_fail git config --get-color no.such.slot "$1"
+}
+
+test_expect_success 'reset' '
+   color "reset" "[m"
+'
+
+test_expect_success 'empty color is empty' '
+   color "" ""
+'
+
+test_expect_success 'attribute before color name' '
+   color "bold red" "[1;31m"
+'
+
+test_expect_success 'color name before attribute' '
+   color "red bold" "[1;31m"
+'
+
+test_expect_success 'attr fg bg' '
+   color "ul blue red" "[4;34;41m"
+'
+
+test_expect_success 'fg attr bg' '
+   color "blue ul red" "[4;34;41m"
+'
+
+test_expect_success 'fg bg attr' '
+   color "blue red ul" "[4;34;41m"
+'
+
+test_expect_success 'fg bg attr...' '
+   color "blue bold dim ul blink reverse" "[1;2;4;5;7;34m"
+'
+
+# note that nobold and nodim are the same code (22)
+test_expect_success 'attr negation' '
+   color "nobold nodim noul noblink noreverse" "[22;24;25;27m"
+'
+
+test_expect_success '"no-" variant of negation' '
+   color "no-bold no-blink" "[22;25m"
+'
+
+test_expect_success 'long color specification' '
+   color "254 255 bold dim ul blink reverse" 
"[1;2;4;5;7;38;5;254;48;5;255m"
+'
+
+test_expect_success 'absurdly long color specification' '
+   color \
+ "#ff #ff bold nobold dim nodim italic noitalic
+  ul noul blink noblink reverse noreverse strike nostrike" \
+ "[1;2;3;4;5;7;9;22;23;24;25;27;29;38;2;255;255;255;48;2;255;255;255m"
+'
+
+test_expect_success '0-7 are aliases for basic ANSI color names' '
+   color "0 7" "[30;47m"
+'
+
+test_expect_success '256 colors' '
+   color "254 bold 255" "[1;38;5;254;48;5;255m"
+'
+
+test_expect_success '24-bit colors' '
+   color "#ff00ff black" "[38;2;255;0;255;40m"
+'
+
+test_expect_success '"normal" yields no color at all"' '
+   color "normal black" "[40m"
+'
+
+test_expect_success '-1 is a synonym for "normal"' '
+   color "-1 black" "[40m"
+'
+
+test_expect_success 'color too small' '
+   invalid_color "-2"
+'
+
+test_expect_success 'color too big' '
+   invalid_color "256"
+'
+
+test_expect_success 'extra character after color number' '
+   invalid_color "3X"
+'
+
+test_expect_success 'extra character after color name' '
+   invalid_color "redX"
+'
+
+test_expect_success 'extra character after attribute' '
+   invalid_color "dimX"
+'
+
+test_expect_success 'unknown color slots are ignored (diff)' '
+   git config color.diff.nosuchslotwilleverbedefined white &&
+   git diff --color
+'
+
+test_expect_success 'unknown color slots are ignored (branch)' '
+   git config color.branch.nosuchslotwilleverbedefined white &&
+   git branch -a
+'
+
+test_expect_success 'unknown color slots are ignored (status)' '
+   git config color.status.nosuchslotwilleverbedefined white &&
+   { git status; ret=$?; } &&
+   case $ret in 0|1) : ok ;; *) false ;; esac
+'
+
+test_done

--
https://github.com/git/git/pull/431

[add-default-config 1/5] add --color option to git config

[Soukaina NAIT HMID]

From: Soukaina NAIT HMID 

Signed-off-by: Soukaina NAIT HMID 
---
 Documentation/git-config.txt | 4 
 builtin/config.c | 1 +
 2 files changed, 5 insertions(+)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 4edd09fc6b074..5d5cd58fdae37 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -179,6 +179,10 @@ See also <>.
specified user.  This option has no effect when setting the
value (but you can use `git config section.variable ~/`
from the command line to let your shell do the expansion).
+--color::
+   Find the color configured for `name` (e.g. `color.diff.new`) and
+   output it as the ANSI color escape sequence to the standard
+   output. 
 
 -z::
 --null::
diff --git a/builtin/config.c b/builtin/config.c
index d13daeeb55927..124a682d50fa8 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -80,6 +80,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, "color", , N_("find the color configured"), 
ACTION_GET_COLOR),
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")),

--
https://github.com/git/git/pull/431

[add-default-config 4/5] add defaults for path/int/bool

[Soukaina NAIT HMID]

From: Soukaina NAIT HMID 

Signed-off-by: Soukaina NAIT HMID 
---
 builtin/config.c  |  12 -
 t/t4026-color2.sh | 129 --
 2 files changed, 11 insertions(+), 130 deletions(-)
 delete mode 100755 t/t4026-color2.sh

diff --git a/builtin/config.c b/builtin/config.c
index 9df2d9c43bcad..eab81c5627091 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -55,6 +55,8 @@ static const char *default_value;
 #define TYPE_BOOL_OR_INT (1<<2)
 #define TYPE_PATH (1<<3)
 
+static char *normalize_value(const char *key, const char *value);
+
 static struct option builtin_config_options[] = {
OPT_GROUP(N_("Config file location")),
OPT_BOOL(0, "global", _global_config, N_("use global config file")),
@@ -256,8 +258,15 @@ static int get_value(const char *key_, const char *regex_)
fwrite(buf->buf, 1, buf->len, stdout);
strbuf_release(buf);
}
-   free(values.items);
 
+   if (values.nr == 0 && default_value) {
+   if(types == TYPE_INT || types == TYPE_BOOL || types == 
TYPE_BOOL_OR_INT || types == TYPE_PATH ) {
+   char* xstr = normalize_value(key, default_value);
+   fwrite(xstr, 1, strlen(xstr), stdout);
+   fwrite("\n", 1, 1, stdout);
+   }
+   }
+   free(values.items);
 free_strings:
free(key);
if (key_regexp) {
@@ -272,6 +281,7 @@ static int get_value(const char *key_, const char *regex_)
return ret;
 }
 
+
 static char *normalize_value(const char *key, const char *value)
 {
if (!value)
diff --git a/t/t4026-color2.sh b/t/t4026-color2.sh
deleted file mode 100755
index 695ce9dd6f8d4..0
--- a/t/t4026-color2.sh
+++ /dev/null
@@ -1,129 +0,0 @@
-#!/bin/sh
-#
-# Copyright (c) 2008 Timo Hirvonen
-#
-
-test_description='Test diff/status color escape codes'
-. ./test-lib.sh
-
-ESC=$(printf '\033')
-color()
-{
-   actual=$(git config --default "$1" --color no.such.slot) &&
-   test "$actual" = "${2:+$ESC}$2"
-}
-
-invalid_color()
-{
-   test_must_fail git config --get-color no.such.slot "$1"
-}
-
-test_expect_success 'reset' '
-   color "reset" "[m"
-'
-
-test_expect_success 'empty color is empty' '
-   color "" ""
-'
-
-test_expect_success 'attribute before color name' '
-   color "bold red" "[1;31m"
-'
-
-test_expect_success 'color name before attribute' '
-   color "red bold" "[1;31m"
-'
-
-test_expect_success 'attr fg bg' '
-   color "ul blue red" "[4;34;41m"
-'
-
-test_expect_success 'fg attr bg' '
-   color "blue ul red" "[4;34;41m"
-'
-
-test_expect_success 'fg bg attr' '
-   color "blue red ul" "[4;34;41m"
-'
-
-test_expect_success 'fg bg attr...' '
-   color "blue bold dim ul blink reverse" "[1;2;4;5;7;34m"
-'
-
-# note that nobold and nodim are the same code (22)
-test_expect_success 'attr negation' '
-   color "nobold nodim noul noblink noreverse" "[22;24;25;27m"
-'
-
-test_expect_success '"no-" variant of negation' '
-   color "no-bold no-blink" "[22;25m"
-'
-
-test_expect_success 'long color specification' '
-   color "254 255 bold dim ul blink reverse" 
"[1;2;4;5;7;38;5;254;48;5;255m"
-'
-
-test_expect_success 'absurdly long color specification' '
-   color \
- "#ff #ff bold nobold dim nodim italic noitalic
-  ul noul blink noblink reverse noreverse strike nostrike" \
- "[1;2;3;4;5;7;9;22;23;24;25;27;29;38;2;255;255;255;48;2;255;255;255m"
-'
-
-test_expect_success '0-7 are aliases for basic ANSI color names' '
-   color "0 7" "[30;47m"
-'
-
-test_expect_success '256 colors' '
-   color "254 bold 255" "[1;38;5;254;48;5;255m"
-'
-
-test_expect_success '24-bit colors' '
-   color "#ff00ff black" "[38;2;255;0;255;40m"
-'
-
-test_expect_success '"normal" yields no color at all"' '
-   color "normal black" "[40m"
-'
-
-test_expect_success '-1 is a synonym for "normal"' '
-   color "-1 black" "[40m"
-'
-
-test_expect_success 'color too small' '
-   invalid_color "-2"
-'
-
-test_expect_success 'color too big' '
-   invalid_color "256"
-'
-
-test_expect_success 'extra character after color number' '
-   invalid_color "3X"
-'
-
-test_expect_success 'extra character after color name' '
-   invalid_color "redX"
-'
-
-test_expect_success 'extra character after attribute' '
-   invalid_color "dimX"
-'
-
-test_expect_success 'unknown color slots are ignored (diff)' '
-   git config color.diff.nosuchslotwilleverbedefined white &&
-   git diff --color
-'
-
-test_expect_success 'unknown color slots are ignored (branch)' '
-   git config color.branch.nosuchslotwilleverbedefined white &&
-   git branch -a
-'
-
-test_expect_success 'unknown color slots are ignored (status)' '
-   git config color.status.nosuchslotwilleverbedefined white &&
-   { git status; ret=$?; } &&
-   

[add-default-config 5/5] fix return code on default + add tests

[Soukaina NAIT HMID]

From: Soukaina NAIT HMID 

Signed-off-by: Soukaina NAIT HMID 
---
 builtin/config.c   |   9 ++-
 t/t9904-default.sh | 232 +
 2 files changed, 238 insertions(+), 3 deletions(-)
 create mode 100755 t/t9904-default.sh

diff --git a/builtin/config.c b/builtin/config.c
index eab81c5627091..29c5f55f27a57 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -261,9 +261,12 @@ static int get_value(const char *key_, const char *regex_)
 
if (values.nr == 0 && default_value) {
if(types == TYPE_INT || types == TYPE_BOOL || types == 
TYPE_BOOL_OR_INT || types == TYPE_PATH ) {
-   char* xstr = normalize_value(key, default_value);
-   fwrite(xstr, 1, strlen(xstr), stdout);
-   fwrite("\n", 1, 1, stdout);
+   if(strlen(default_value)) {
+   char* xstr = normalize_value(key, 
default_value);
+   fwrite(xstr, 1, strlen(xstr), stdout);
+   fwrite("\n", 1, 1, stdout);
+   ret = 0;
+   }
}
}
free(values.items);
diff --git a/t/t9904-default.sh b/t/t9904-default.sh
new file mode 100755
index 0..8e838f512298b
--- /dev/null
+++ b/t/t9904-default.sh
@@ -0,0 +1,232 @@
+#!/bin/sh
+
+test_description='Test default color/boolean/int/path'
+. ./test-lib.sh
+
+boolean()
+{
+   slot=$([ "$#" == 3 ] && echo $3 || echo "no.such.slot") &&
+   actual=$(git config --default "$1" --bool "$slot") &&
+   test "$actual" = "$2"
+}
+
+invalid_boolean()
+{
+   slot=$([ "$#" == 2 ] && echo $2 || echo "no.such.slot") &&
+   test_must_fail git config --default "$1" --bool "$slot"
+}
+
+test_expect_success 'empty value for boolean' '
+   invalid_boolean ""
+'
+
+test_expect_success 'true' '
+   boolean "true" "true"
+'
+
+test_expect_success '1 is true' '
+   boolean "1" "true"
+'
+
+test_expect_success 'non-zero is true' '
+   boolean "5312" "true"
+'
+
+test_expect_success 'false' '
+   boolean "false" "false"
+'
+
+test_expect_success '0 is false' '
+   boolean "0" "false"
+'
+
+test_expect_success 'invalid value' '
+   invalid_boolean "ab"
+'
+
+test_expect_success 'existing slot has priority = true' '
+   git config bool.value true &&
+   boolean "false" "true" "bool.value"
+'
+
+test_expect_success 'existing slot has priority = false' '
+   git config bool.value false &&
+   boolean "true" "false" "bool.value"
+'
+
+int()
+{
+   slot=$([ "$#" == 3 ] && echo $3 || echo "no.such.slot") &&
+   actual=$(git config --default "$1" --int "$slot") &&
+   test "$actual" = "$2"
+}
+
+invalid_int()
+{
+   slot=$([ "$#" == 2 ] && echo $2 || echo "no.such.slot") &&
+   test_must_fail git config "$1" --int "$slot"
+}
+
+test_expect_success 'empty value for int' '
+   invalid_int "" ""
+'
+
+test_expect_success 'positive' '
+   int "12345" "12345"
+'
+
+test_expect_success 'negative' '
+   int "-679032" "-679032"
+'
+
+test_expect_success 'invalid value' '
+   invalid_int "abc"
+'
+test_expect_success 'existing slot has priority = 123' '
+   git config int.value 123 &&
+   int "666" "123" "int.value"
+'
+
+test_expect_success 'existing slot with bad value' '
+   git config int.value abc &&
+   invalid_int "123" "int.value"
+'
+
+path()
+{
+   slot=$([ "$#" == 3 ] && echo $3 || echo "no.such.slot") &&
+   actual=$(git config --default "$1" --path "$slot") &&
+   test "$actual" = "$2"
+}
+
+invalid_path()
+{
+   slot=$([ "$#" == 2 ] && echo $2 || echo "no.such.slot") &&
+   test_must_fail git config "$1" --path "$slot"
+}
+
+test_expect_success 'empty path is invalid' '
+   invalid_path "" ""
+'
+
+test_expect_success 'valid path' '
+   path "/aa/bb/cc" "/aa/bb/cc"
+'
+
+test_expect_success 'existing slot has priority = /to/the/moon' '
+   git config path.value /to/the/moon &&
+   path "/to/the/sun" "/to/the/moon" "path.value"
+'
+ESC=$(printf '\033')
+
+color()
+{
+   slot=$([ "$#" == 3 ] && echo $3 || echo "no.such.slot") &&
+   actual=$(git config --default "$1" --color "$slot" ) &&
+   test "$actual" = "${2:+$ESC}$2"
+}
+
+invalid_color()
+{
+   slot=$([ "$#" == 2 ] && echo $2 || echo "no.such.slot") &&
+   test_must_fail git config --default "$1" --color "$slot"
+}
+
+test_expect_success 'reset' '
+   color "reset" "[m"
+'
+
+test_expect_success 'empty color is empty' '
+   color "" ""
+'
+
+test_expect_success 'attribute before color name' '
+   color "bold red" "[1;31m"
+'
+
+test_expect_success 'color name before attribute' '
+   color "red bold" "[1;31m"
+'
+
+test_expect_success 'attr fg bg' '
+   color "ul blue red" "[4;34;41m"
+'
+
+test_expect_success 'fg attr bg' '
+   color "blue 

[add-default-config 2/5] adding default to color

[Soukaina NAIT HMID]

From: Soukaina NAIT HMID 

Signed-off-by: Soukaina NAIT HMID 
---
 builtin/config.c | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/builtin/config.c b/builtin/config.c
index 124a682d50fa8..9df2d9c43bcad 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -30,6 +30,7 @@ static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;
 static int show_origin;
+static const char *default_value;
 
 #define ACTION_GET (1<<0)
 #define ACTION_GET_ALL (1<<1)
@@ -47,6 +48,7 @@ static int show_origin;
 #define ACTION_GET_COLOR (1<<13)
 #define ACTION_GET_COLORBOOL (1<<14)
 #define ACTION_GET_URLMATCH (1<<15)
+#define ACTION_GET_COLORORDEFAULT (1<<16)
 
 #define TYPE_BOOL (1<<0)
 #define TYPE_INT (1<<1)
@@ -80,12 +82,13 @@ 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, "color", , N_("find the color configured"), 
ACTION_GET_COLOR),
+   OPT_BIT(0, "color", , N_("find the color configured"), 
ACTION_GET_COLORORDEFAULT),
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")),
OPT_BOOL(0, "includes", _includes_opt, N_("respect include 
directives on lookup")),
OPT_BOOL(0, "show-origin", _origin, N_("show origin of config 
(file, standard input, blob, command line)")),
+   OPT_STRING(0, "default", _value, N_("default-value"), N_("sets 
default for bool/int/path/color when no value is returned from config")),
OPT_END(),
 };
 
@@ -331,6 +334,11 @@ static void get_color(const char *var, const char 
*def_color)
fputs(parsed_color, stdout);
 }
 
+static void get_color_default(const char *var)
+{
+   get_color(var, default_value);
+}
+
 static int get_colorbool_found;
 static int get_diff_color_found;
 static int get_color_ui_found;
@@ -726,6 +734,10 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
check_argc(argc, 1, 2);
get_color(argv[0], argv[1]);
}
+   else if (actions == ACTION_GET_COLORORDEFAULT) {
+   check_argc(argc, 1, 1);
+   get_color_default(argv[0]);
+   }
else if (actions == ACTION_GET_COLORBOOL) {
check_argc(argc, 1, 2);
if (argc == 2)

--
https://github.com/git/git/pull/431

Re: [PATCH] config: added --expiry-date type support

[Jeff King]

On Sun, Nov 12, 2017 at 12:19:35PM +, Haaris wrote:

> ---

Hi, and welcome to the list. Thanks for working on this (for those of
you on the list, this was one of the tasks at the hackathon this
weekend).

Kevin already mentioned a few things about the commit message, which I
agree with.

>  builtin/config.c   | 11 ++-
>  config.c   |  9 +
>  config.h   |  1 +
>  t/t1300-repo-config.sh | 25 +

It's great that there are new tests. We'll probably need some
documentation, too (especially users will need to know what the output
format means).

> @@ -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")),

We seem to use both "expire" and "expiry" throughout the code and in
user-facing bits (e.g., "gc.reflogExpire" and "gc.logExpiry"). I don't
have a real preference for one versus the other. I just mention it since
whatever we choose here will be locked in to the interface forever.

> @@ -159,6 +161,12 @@ 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 = malloc(sizeof(*t));
> + if(git_config_expiry_date(, key_, value_) < 0)
> + return -1;
> + strbuf_addf(buf, "%"PRItime, *t);
> + free((timestamp_t *)t);
>   } else if (value_) {

Since we only need the timestamp variable within this block, we don't
need to use a pointer. We can just do something like:

  } else if (types == TYPE_EXPIRY_DATE) {
timestamp_t t;
if (git_config_expiry_date(, key_, value_) < 0)
return -1;
strbuf_addf(buf, "%"PRItime", t);
  }

Note that your new git_config_expiry_date would want to take just a
regular pointer, rather than a pointer-to-pointer. I suspect you picked
that up from git_config_pathname(). It needs the double pointer because
it's storing a string (which is itself a pointer), but we don't need
that here.

> @@ -273,12 +281,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);

This makes sense. The expiration values we get from the user are
typically relative (like "2.weeks"), so it wouldn't make sense to store
the absolute value we get from applying that relative offset to the
current time.

> diff --git a/config.c b/config.c
> index 903abf9533b18..caa2fd5fb6915 100644
> --- a/config.c
> +++ b/config.c
> @@ -990,6 +990,15 @@ 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))
> + die(_("failed to parse date_string in: '%s'"), value);
> + return 0;
> +}

I was surprised that we don't already have a function that does this,
since we parse expiry config elsewhere. We do, but it's just local to
builtin/reflog.c. So perhaps as a preparatory step we should add this
function and convert reflog.c to use it, dropping its custom
parse_expire_cfg_value().

What's the purpose of the "!!" before parse_expiry_date()? The usual
idiom for that to normalize a non-zero value into "1", but we don't care
here. I think just:

  if (parse_expiry_date(value, timestamp))
die(...);

would be sufficient.

> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> index 364a537000bbb..59a35be89e511 100755
> --- a/t/t1300-repo-config.sh
> +++ b/t/t1300-repo-config.sh
> @@ -901,6 +901,31 @@ test_expect_success 'get --path barfs on boolean 
> variable' '
>   test_must_fail git config 

Re: [PATCH] config: added --expiry-date type support

[Jeff King]

On Sun, Nov 12, 2017 at 02:55:53PM +0100, Kevin Daudt wrote:

> What I'm also missing is a motivation on why you added this option,
> which should be part of your commit message. As far as I know, there is
> currently no config setting that expects a date format.

This patch came from submitGit, and there's a bit more at:

  https://github.com/git/git/pull/433

(though obviously that informatoin should go into the commit message).

We do parse expiration dates from config in a few places, like
gc.reflogexpire, etc. There's no way for a script using git-config to do
the same (either adding an option specific to the script, or trying to
do some analysis on gc.reflogexpire).

-Peff

Re: should "git bisect" support "git bisect next?"

[Christian Couder]

On Sun, Nov 12, 2017 at 2:43 AM, Junio C Hamano  wrote:
> Theodore Ts'o  writes:
>
>> On Sat, Nov 11, 2017 at 11:38:23PM +0900, Junio C Hamano wrote:
>>>
>>> Thanks for saving me time to explain why 'next' is still a very
>>> important command but the end users do not actually need to be
>>> strongly aware of it, because most commands automatically invokes it
>>> as their final step due to the importance of what it does ;-)
>>
>> This reminds me; is there a way to suppress it because I'm about to
>> give a large set of good and bit commits (perhaps because I'm
>> replaying part of a git biset log, minus one or two lines that are
>> suspected of being bogus thanks to flaky reproduction), and so there's
>> no point having git bisect figure the "next" commit to try until I'm
>> done giving it a list of good/bad commits?
>
> It is surprising that I've never heard of this idea, but I think
> that it is an excellent one.
>
> When the user knows what bad and good commits in what sequence will
> be fed to the command (i.e. replaying a saved output of "git bisect
> log"), ideally we would want to "plug" the auto-next processing, and
> just mark good and bad in refs/bisect/* without doing anything other
> than creating these refs, and then run a "next" before giving the
> control back to present the final working tree to be tested by the
> user.  That would save the cost of intermediate checkouts but also
> the cost of extra merge-base computation that is done to catch the
> case where the user gave a good commit that is an ancestor of a bad
> commit by mistake.

Yeah I agree that it might be something interesting for the user to do.
But in this case the sequence in which you give the good and the bad
commits is not important.
Only the last bad commit and the set of good commits that were given
are important.

If you can get that and pass it to 'git bisect start' then you will
avoid all the intermediate computation and actually start from the
state you want.

> I think that the output of "git bisect log" was designed to be just
> an executable shell script that the user can edit (the edit is
> mostly designed to make it possible: "I know I screwed up in this
> step, so I change its 'bad' to 'good' and remove the remaining
> lines") and just execute it.  Which makes the simplest approach that
> would first come to my mind not work very well, unfortunately.
>
> The "simplest approach" would teach the "--no-autonext"
> option to "git bisect good" and "git bisect bad" and skip
> the call to bisect_auto_next in bisect_state when it is
> given.  Then update the output from "git bisect log" to add
> that option to all good/bad commands, and then add an
> explicit "git bisect next" at the end.  This won't work well
> because it is likely that with the "remove the remaining
> lines" step, it is likely that the user would remove the
> final "bisect next".
>
> A workable alternative approach is to teach "git bisect replay" to
> be more intelligent.  Right now, I do not think it does anything
> more than what happens by an execution of the input file with a
> shell, but "replay" should be able to read a single step ahead in
> the command sequence while doing its step-by-step execution, and
> when it notices that it is about to run "bisect good" or "bisect
> bad", and if the command to be run after that is also one of these
> two, it can decide to skip the auto-next processing in the current
> step.  It shouldn't need any new "--no-auto-next" option (I am not
> saying that adding the option is bad--in fact, I think it is a good
> addition for expert users; I am just saying that the approach to
> make "replay" smarter does not require it).

To automate that one could start with a dirty oneliner like this:

git bisect start $(git bisect log | perl -ne '$b = $1 if m/# bad:
\[(.*)\]/; push @g, $1 if m/# good: \[(.*)\]/; END { print $b . " ".
join(" ", @g) . "\n"; }')

So yeah we could add an option to "git replay" that would do that.

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

[Jeff King]

On Sat, Nov 11, 2017 at 07:28:58PM +, gennady.kup...@gmail.com wrote:

> From: Gennady Kupava 
> 
> Signed-off-by: Gennady Kupava 

Thanks, and welcome to the list.

> ---
> One of the tasks siggested in scope of 'Git sprint weekend'.
> Couple of chioces made here:

These kinds of choices/caveats (along with the motivation for the patch)
should go into the commit message so people running "git log" later can
see them.

>  1. Use constant instead of NULL to indicate default trace level, this just
> makes everything simpler.

I think this makes sense, as we were translating NULL into this default
struct behind the scenes already. As we discussed off-list, this does
mean that you can no longer do:

  trace_printf_key(NULL, "foo");

to send to the default key, and instead must do:

  trace_printf("foo");

The second one has always been the preferred way of spelling it, but the
first one happened to work. So the only fallout would be if somebody is
using the non-preferred method, they'd now segfault. There aren't any
such calls in the current code base, though, and I find it rather
unlikely that there would be new instances in topics other people are
working on.

I think it may be worth splitting that out into a separate preparatory
patch to make it more clear what's going on (especially if our
assumption turns out wrong and somebody does end up tracing a problem
back to it).

>  2. Move just enough from implementation to header, to be able to do check
> before calling actual functions.

Makes sense.

>  3. Most controversail: do not support optimization for older
> compilers not supporting variadic templates in macroses. Problem
> is that theoretically someone may write some complicated trace
> while would work in 'modern' compiler and be too slow in more
> 'classic' compilers, however expectation is that risk of that is
> quite low and 'classic' compilers will go away eventually.

I think this is probably acceptable. I know we've discussed elsewhere
recently how common such compilers actually are, and whether we could
give up on them altogether.

If we wanted to, we could support that case by using inline functions in
the header (though it does make me wonder if compilers that old also do
not actually end up inlining).

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).

> diff --git a/trace.c b/trace.c
> index 7508aea02..180ee3b00 100644
> --- a/trace.c
> +++ b/trace.c
> @@ -25,26 +25,14 @@
>  #include "cache.h"
>  #include "quote.h"
>  
> -/*
> - * "Normalize" a key argument by converting NULL to our trace_default,
> - * and otherwise passing through the value. All caller-facing functions
> - * should normalize their inputs in this way, though most get it
> - * for free by calling get_trace_fd() (directly or indirectly).
> - */
> -static void normalize_trace_key(struct trace_key **key)
> -{
> - static struct trace_key trace_default = { "GIT_TRACE" };
> - if (!*key)
> - *key = _default;
> -}
> +struct trace_key trace_default_key = { TRACE_KEY_PREFIX, 0, 0, 0 };

Makes sense. We no longer auto-normalize, but just expect the
appropriate functions to pass a pointer to the default key themselves.

> +struct trace_key trace_perf_key = TRACE_KEY_INIT(PERFORMANCE);

This change was unexpected, but I think you just moved this up to keep
all the pre-declared structs together. Makes sense.

>  /* Get a trace file descriptor from "key" env variable. */
>  static int get_trace_fd(struct trace_key *key)
>  {
>   const char *trace;
>  
> - normalize_trace_key();
> -
>   /* don't open twice */
>   if (key->initialized)
>   return key->fd;

And this and similar changes to drop normalize_trace_key() all make
sense with the earlier hunk. Good.

> @@ -341,7 +324,7 @@ void trace_repo_setup(const char *prefix)
>  
>  int trace_want(struct trace_key *key)
>  {
> - return !!get_trace_fd(key);
> +   return !!get_trace_fd(key);
>  }

This line accidentally swapped out the tab for spaces.

> diff --git a/trace.h b/trace.h
> index 179b249c5..64326573a 100644
> --- a/trace.h
> +++ b/trace.h
> @@ -4,6 +4,8 @@
>  #include "git-compat-util.h"
>  #include "strbuf.h"
>  
> +#define TRACE_KEY_PREFIX "GIT_TRACE"
> [..]
> -#define TRACE_KEY_INIT(name) { "GIT_TRACE_" #name, 0, 0, 0 }
> +#define TRACE_KEY_INIT(name) { TRACE_KEY_PREFIX "_" #name, 0, 0, 0 }

I see you pulled this out so you could use TRACE_KEY_PREFIX elsewhere
for the default key. Yes, the default key and the prefix do happen to be
related in that way, but I actually think it was a bit less obfuscated
the original way, even it repeated the string "GIT_TRACE" twice.

> +#define trace_pass(key) ((key)->fd || !(key)->initialized)

Factoring this out makes sense, since we have to repeat it in each
macro.

Speaking of macros, one side 

Re: [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming

[Kaartic Sivaraam]

On Mon, 2017-11-06 at 11:30 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam  writes:
> 
> No {} around a single statement block of "if", especially when there
> is no "else" that has multi-statement block that needs {}.
> 

The code has changed a little since v3 so this if has been replaced
with a switch case.


> > +   switch (res) {
> > +   case BRANCH_EXISTS_NO_FORCE:
> > +   strbuf_addf(error_msg, "%s", (!old_branch_exists) ? 
> > connector_string : "");
> > +   strbuf_addf(error_msg,_("branch '%s' already exists"), 
> > newname);
> > +   break;
> 
> The case arms and their statements are indented by one level too much.
> The lines are getting overlong.  Find a good place to split, e.g.
> 
>   strbuf_addf(error_msg, "%s",
>   !old_branch_exists ? connector_string : "");
> 
> Leave a single SP after each "," in an arguments list.
> 

Fixed these.


> As Eric pointed out, this certainly smells like a sentence lego that
> we would be better off without.
> 

As stated in that mail,  I've replaced the connector " and " with "; "
as it seemed to be the most simple way to overcome the issue, at least
in my opinion. In case there's any better way to fix this let me know.


> >  static void copy_or_rename_branch(const char *oldname, const char 
> > *newname, int copy, int force)
> >  {
> > struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = 
> > STRBUF_INIT;
> > struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
> > int recovery = 0;
> > +   struct strbuf error_msg = STRBUF_INIT, empty = STRBUF_INIT;
> > +   enum branch_validation_result res;
> >  
> > if (!oldname) {
> > if (copy)
> > @@ -471,15 +502,13 @@ static void copy_or_rename_branch(const char 
> > *oldname, const char *newname, int
> > die(_("cannot rename the current branch while not on 
> > any."));
> > }
> >  
> > -   if (strbuf_check_branch_ref(, oldname)) {
> > +   if (strbuf_check_branch_ref(, oldname) && ref_exists(oldref.buf))
> > +   {
> 
> Opening brace { that begins a block comes at the end of the line
> that closes the condition of "if"; if you found that your line is
> overlong, perhaps do it like so instead:
> 
>   if (strbuf_check_branch_ref(, oldname) &&
>   ref_exists(oldref.buf)) {

This part changed too. Anyways thanks for the detailed review :-)

-- 
Kaartic

Re: [PATCH] config: added --expiry-date type support

[Kevin Daudt]

On Sun, Nov 12, 2017 at 12:19:35PM +, Haaris wrote:
> ---
>  builtin/config.c   | 11 ++-
>  config.c   |  9 +
>  config.h   |  1 +
>  t/t1300-repo-config.sh | 25 +
>  4 files changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/config.c b/builtin/config.c
> index d13daeeb55927..41cd9f5ca7cde 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,12 @@ 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 = malloc(sizeof(*t));
> + if(git_config_expiry_date(, key_, value_) < 0)
> + return -1;
> + strbuf_addf(buf, "%"PRItime, *t);
> + free((timestamp_t *)t);
>   } else if (value_) {
>   strbuf_addstr(buf, value_);
>   } else {
> @@ -273,12 +281,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/config.c b/config.c
> index 903abf9533b18..caa2fd5fb6915 100644
> --- a/config.c
> +++ b/config.c
> @@ -990,6 +990,15 @@ 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))
> + die(_("failed to parse date_string in: '%s'"), value);
> + return 0;
> +}
> +
>  static int git_default_core_config(const char *var, const char *value)
>  {
>   /* This needs a better name */
> diff --git a/config.h b/config.h
> index a49d264416225..2d127d9d23c90 100644
> --- a/config.h
> +++ b/config.h
> @@ -58,6 +58,7 @@ extern int git_config_bool_or_int(const char *, const char 
> *, int *);
>  extern int git_config_bool(const char *, const char *);
>  extern int git_config_string(const char **, const char *, const char *);
>  extern int git_config_pathname(const char **, const char *, const char *);
> +extern int git_config_expiry_date(timestamp_t **, const char *, const char 
> *);
>  extern int git_config_set_in_file_gently(const char *, const char *, const 
> char *);
>  extern void git_config_set_in_file(const char *, const char *, const char *);
>  extern int git_config_set_gently(const char *, const char *);
> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> index 364a537000bbb..59a35be89e511 100755
> --- a/t/t1300-repo-config.sh
> +++ b/t/t1300-repo-config.sh
> @@ -901,6 +901,31 @@ test_expect_success 'get --path barfs on boolean 
> variable' '
>   test_must_fail git config --get --path path.bool
>  '
>  
> +test_expect_success 'get --expiry-date' '
> + cat >.git/config <<-\EOF &&
> + [date]
> + valid1 = "Fri Jun 4 15:46:55 2010"
> + valid2 = "2017/11/11 11:11:11PM"
> + valid3 = "2017/11/10 09:08:07 PM"
> + valid4 = "never"
> + invalid1 = "abc"
> + EOF
> + cat >expect <<-\EOF &&
> + 1275666415
> + 1510441871
> + 1510348087
> + 0
> + EOF
> + {
> + git config --expiry-date date.valid1 &&
> + git config --expiry-date date.valid2 &&
> + git config --expiry-date date.valid3 &&

[PATCH v1 2/2] worktree: make add dwim

[Thomas Gummerer]

Currently git worktree add creates a new branch, that matches the HEAD
of whichever worktree we were on when calling "git worktree add".

Add a --guess flag to git worktree add, that makes it behave like the
dwim machinery in 'git checkout ', i.e. check if the new
branch name uniquely matches the branch name of a remote tracking
branch, and if so check out that branch, and set the upstream to the
remote tracking branch.

Signed-off-by: Thomas Gummerer 
---

I'm a bit torn about hiding his behind an additional flag in git
worktree add or not.  I would like to have the feature without the
additional flag, but it might break some peoples expectations, so
dunno.

 builtin/worktree.c  | 14 +-
 t/t2025-worktree-add.sh | 70 +
 2 files changed, 83 insertions(+), 1 deletion(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 7b9307aa58..5740d1f8a3 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "checkout.h"
 #include "config.h"
 #include "builtin.h"
 #include "dir.h"
@@ -341,6 +342,7 @@ static int add(int ac, const char **av, const char *prefix)
const char *new_branch_force = NULL;
char *path;
const char *branch;
+   int dwim_new_branch = 0;
struct option options[] = {
OPT__FORCE(, N_("checkout  even if already 
checked out in other worktree")),
OPT_STRING('b', NULL, _branch, N_("branch"),
@@ -350,6 +352,7 @@ static int add(int ac, const char **av, const char *prefix)
OPT_BOOL(0, "detach", , N_("detach HEAD at named 
commit")),
OPT_BOOL(0, "checkout", , N_("populate the new 
working tree")),
OPT_BOOL(0, "lock", _locked, N_("keep the new working 
tree locked")),
+   OPT_BOOL(0, "guess", _new_branch, N_("checkout upstream 
branch if there's a unique match")),
OPT_END()
};
 
@@ -363,6 +366,7 @@ static int add(int ac, const char **av, const char *prefix)
 
path = prefix_filename(prefix, av[0]);
branch = ac < 2 ? "HEAD" : av[1];
+   dwim_new_branch = ac < 2 ? dwim_new_branch : 0;
 
if (!strcmp(branch, "-"))
branch = "@{-1}";
@@ -387,13 +391,21 @@ static int add(int ac, const char **av, const char 
*prefix)
}
 
if (opts.new_branch) {
+   struct object_id oid;
+   const char *remote;
struct child_process cp = CHILD_PROCESS_INIT;
+
+   remote = unique_tracking_name(opts.new_branch, );
+
cp.git_cmd = 1;
argv_array_push(, "branch");
if (opts.force_new_branch)
argv_array_push(, "--force");
argv_array_push(, opts.new_branch);
-   argv_array_push(, branch);
+   if (dwim_new_branch && remote)
+   argv_array_push(, remote);
+   else
+   argv_array_push(, branch);
if (run_command())
return -1;
branch = opts.new_branch;
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index b5c47ac602..b37c279787 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -6,6 +6,16 @@ test_description='test git worktree add'
 
 . "$TEST_DIRECTORY"/lib-rebase.sh
 
+# Is branch "refs/heads/$1" set to pull from "$2/$3"?
+test_branch_upstream () {
+   printf "%s\n" "$2" "refs/heads/$3" >expect.upstream &&
+   {
+   git config "branch.$1.remote" &&
+   git config "branch.$1.merge"
+   } >actual.upstream &&
+   test_cmp expect.upstream actual.upstream
+}
+
 test_expect_success 'setup' '
test_commit init
 '
@@ -314,4 +324,64 @@ test_expect_success 'rename a branch under bisect not 
allowed' '
test_must_fail git branch -M under-bisect bisect-with-new-name
 '
 
+test_expect_success 'git worktree add does not dwim' '
+   test_when_finished rm -rf repo_a &&
+   test_when_finished rm -rf repo_b &&
+   test_when_finished rm -rf foo &&
+   git init repo_a &&
+   (
+   cd repo_a &&
+   test_commit a_master &&
+   git checkout -b foo &&
+   test_commit a_foo
+   ) &&
+   git init repo_b &&
+   (
+   cd repo_b &&
+   test_commit b_master &&
+   git remote add repo_a ../repo_a &&
+   git config remote.repo_a.fetch \
+   "+refs/heads/*:refs/remotes/other_a/*" &&
+   git fetch --all &&
+   git worktree add ../foo
+   ) &&
+   (
+   cd foo &&
+   ! test_branch_upstream foo repo_a foo &&
+   git rev-parse other_a/foo >expect &&
+   git rev-parse foo >actual &&
+   ! test_cmp expect actual
+   )
+'
+
+test_expect_success 'git 

[PATCH v1 1/2] checkout: factor out functions to new lib file

[Thomas Gummerer]

Factor the functions out, so they can be re-used from other places.  In
particular these functions will be re-used in builtin/worktree.c to make
git worktree add dwim more.

While there add a description to the function.

Signed-off-by: Thomas Gummerer 
---

I'm not sure where these functions should go ideally, they don't seem
to fit in any existing library file, so I created a new one for now.
Any suggestions for a better home are welcome!

 Makefile   |  1 +
 builtin/checkout.c | 41 +
 checkout.c | 43 +++
 checkout.h | 14 ++
 4 files changed, 59 insertions(+), 40 deletions(-)
 create mode 100644 checkout.c
 create mode 100644 checkout.h

diff --git a/Makefile b/Makefile
index cd75985991..8d603c7443 100644
--- a/Makefile
+++ b/Makefile
@@ -757,6 +757,7 @@ LIB_OBJS += branch.o
 LIB_OBJS += bulk-checkin.o
 LIB_OBJS += bundle.o
 LIB_OBJS += cache-tree.o
+LIB_OBJS += checkout.o
 LIB_OBJS += color.o
 LIB_OBJS += column.o
 LIB_OBJS += combine-diff.o
diff --git a/builtin/checkout.c b/builtin/checkout.c
index fc4f8fd2ea..9e1cfd10b3 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1,5 +1,6 @@
 #include "builtin.h"
 #include "config.h"
+#include "checkout.h"
 #include "lockfile.h"
 #include "parse-options.h"
 #include "refs.h"
@@ -874,46 +875,6 @@ static int git_checkout_config(const char *var, const char 
*value, void *cb)
return git_xmerge_config(var, value, NULL);
 }
 
-struct tracking_name_data {
-   /* const */ char *src_ref;
-   char *dst_ref;
-   struct object_id *dst_oid;
-   int unique;
-};
-
-static int check_tracking_name(struct remote *remote, void *cb_data)
-{
-   struct tracking_name_data *cb = cb_data;
-   struct refspec query;
-   memset(, 0, sizeof(struct refspec));
-   query.src = cb->src_ref;
-   if (remote_find_tracking(remote, ) ||
-   get_oid(query.dst, cb->dst_oid)) {
-   free(query.dst);
-   return 0;
-   }
-   if (cb->dst_ref) {
-   free(query.dst);
-   cb->unique = 0;
-   return 0;
-   }
-   cb->dst_ref = query.dst;
-   return 0;
-}
-
-static const char *unique_tracking_name(const char *name, struct object_id 
*oid)
-{
-   struct tracking_name_data cb_data = { NULL, NULL, NULL, 1 };
-   cb_data.src_ref = xstrfmt("refs/heads/%s", name);
-   cb_data.dst_oid = oid;
-   for_each_remote(check_tracking_name, _data);
-   free(cb_data.src_ref);
-   if (cb_data.unique)
-   return cb_data.dst_ref;
-   free(cb_data.dst_ref);
-   return NULL;
-}
-
 static int parse_branchname_arg(int argc, const char **argv,
int dwim_new_local_branch_ok,
struct branch_info *new,
diff --git a/checkout.c b/checkout.c
new file mode 100644
index 00..a9cf378453
--- /dev/null
+++ b/checkout.c
@@ -0,0 +1,43 @@
+#include "cache.h"
+
+#include "remote.h"
+
+struct tracking_name_data {
+   /* const */ char *src_ref;
+   char *dst_ref;
+   struct object_id *dst_oid;
+   int unique;
+};
+
+static int check_tracking_name(struct remote *remote, void *cb_data)
+{
+   struct tracking_name_data *cb = cb_data;
+   struct refspec query;
+   memset(, 0, sizeof(struct refspec));
+   query.src = cb->src_ref;
+   if (remote_find_tracking(remote, ) ||
+   get_oid(query.dst, cb->dst_oid)) {
+   free(query.dst);
+   return 0;
+   }
+   if (cb->dst_ref) {
+   free(query.dst);
+   cb->unique = 0;
+   return 0;
+   }
+   cb->dst_ref = query.dst;
+   return 0;
+}
+
+const char *unique_tracking_name(const char *name, struct object_id *oid)
+{
+   struct tracking_name_data cb_data = { NULL, NULL, NULL, 1 };
+   cb_data.src_ref = xstrfmt("refs/heads/%s", name);
+   cb_data.dst_oid = oid;
+   for_each_remote(check_tracking_name, _data);
+   free(cb_data.src_ref);
+   if (cb_data.unique)
+   return cb_data.dst_ref;
+   free(cb_data.dst_ref);
+   return NULL;
+}
diff --git a/checkout.h b/checkout.h
new file mode 100644
index 00..9272fe1449
--- /dev/null
+++ b/checkout.h
@@ -0,0 +1,14 @@
+#ifndef CHECKOUT_H
+#define CHECKOUT_H
+
+#include "git-compat-util.h"
+#include "cache.h"
+
+/*
+ * Check if the branch name uniquely matches a branch name on a remote
+ * tracking branch.  Return the name of the remote if such a branch
+ * exists, NULL otherwise.
+ */
+extern const char *unique_tracking_name(const char *name, struct object_id 
*oid);
+
+#endif /* CHECKOUT_H */
-- 
2.15.0.403.gc27cc4dac6

Re: [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation

[Kaartic Sivaraam]

On Mon, 2017-11-06 at 11:24 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam  writes:
> 
> We usually use the word "gently" to when we enhance an operation
> that used to always die on failure.  When there are tons of callsite
> to the original operation F(), we introduce F_gently() variant and
> do something like
> 
>   F(...)
>   {
>   if (F_gently(...))
>   die(...);
>   }
> 
> so that the callers do not have to change.  If there aren't that
> many, it is OK to change the function signature of F() to tell it
> not to die without adding a new F_gently() function, which is the
> approach more appropriate for this change.  The extra parameter used
> for that purpose should be called "gently", perhaps.
> 

Good suggestion, wasn't aware of it before. Renamed it.


> > +   if(ref_exists(ref->buf))
> > +   return BRANCH_EXISTS;
> > +   else
> > +   return BRANCH_DOESNT_EXIST;
> 
> Always have SP between "if" (and other keyword like "while") and its
> condition.
>
> For this one, however, this might be easier to follow:
> 
>   return ref_exists(ref->buf) ? BRANCH_EXISTS : BRANCH_DOESNT_EXIST;
> 

Done.


> The names of the enum values may need further thought.  They must
> clearly convey two things, in addition to what kind of status they
> represent; the current names only convey the status.  From the names
> of these values, it must be clear that they are part of the same
> enum (e.g. by sharing a common prefix), and also from the names of
> these values, it must be clear which ones are error conditions and
> which are not, without knowing their actual values.  A reader cannot
> immediately tell from "BRANCH_EXISTS" if that is a good thing or
> not.
> 

I've added the prefix of "VALIDATION_{FAIL,PASS}" as appropriate to the
enum values. This made the names to have the form,

VALIDATION_(kind of status)_(reason)

This made the names a bit long but I couldn't come up with a better
prefix for now. Any suggestions are welcome.


Thanks for the detailed review!

---
Kaartic

Re: [RFC PATCH v3 2/4] branch: re-order function arguments to group related arguments

[Kaartic Sivaraam]

On Mon, 2017-11-06 at 11:06 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam  writes:
> 
> > From: Kaartic Sivaraam 
> > 
> > The ad-hoc patches to add new arguments to a function when needed
> > resulted in the related arguments not being close to each other.
> > This misleads the person reading the code to believe that there isn't
> > much relation between those arguments while it's not the case.
> 
> I do not get what this wants to say.  "I am sending this ad-hoc
> patch that scrambles the order of parameters for no real reason" is
> certainly not how you are selling this step.
> 
> > So, re-order the arguments to keep the related arguments close to each
> > other.
> 
> This sentence (without "So,", obviously, because I do not get the
> previous paragraph) I do understand and agree with as a goal.
> 

I've tried to improve it, does the following paragraph sound clear
enough?

branch: group related arguments of create_branch()

New arguments were added to create_branch() whenever the need
arised and they were added to tail of the argument list. This
resulted in the related arguments not being close to each other.
This misleads the person reading the code to believe that
there isn't much relation between those arguments while it is
not the case.

So, re-order the arguments to keep the related arguments close
to each other.


> I think the only two things that should be kept together are "force"
> and "clobber_head_ok" because the previous 1/4 changed the meaning
> of "clobber_head" to "it is OK if I am renaming the currently
> checked-out branch", i.e. closer to what "force" means.
> 
> I certainly would not mind the order used in the result of this
> patch (in other words, if somebody posted a patch to add
> create_branch() function to the codebase that lacked it, with its
> parameters listed in the order this patch uses, I wouldn't
> complain), but it would have equally been OK if "reflog" and "force"
> were swapped without making any other change this patch makes.
> 

Makes sense. The unwanted shuffling was a consequence of my attempt to
see if the signature of the function did change when the position of
the 'enum' was changed. It seems there isn't change in its signature as
it is possible to use integers for enums and vice versa due to liberal
checking for misuses.

I've changed the signature back to keep alone "force" and
"clobber_head_ok" together.


Thanks,
Kaartic

[PATCH v2] gpg-interface: strip CR chars for Windows gpg2

[Jerzy Kozera]

This fixes the issue with newlines being \r\n and not being displayed
correctly when using gpg2 for Windows, as reported at
https://github.com/git-for-windows/git/issues/1249

Issues with non-ASCII characters remain for further investigation.

Signed-off-by: Jerzy Kozera 
---

Addressed comments by Junio C Hamano (check for following \n, and
updated the commit description).

 gpg-interface.c | 27 ++-
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 4feacf16e5..ab592af7f2 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -145,6 +145,20 @@ const char *get_signing_key(void)
return git_committer_info(IDENT_STRICT|IDENT_NO_DATE);
 }
 
+/* Strip CR from the CRLF line endings, in case we are on Windows. */
+static void strip_cr(struct strbuf *buffer, size_t bottom) {
+   size_t i, j;
+   for (i = j = bottom; i < buffer->len; i++)
+   if (!(i < buffer->len - 1 &&
+   buffer->buf[i] == '\r' &&
+   buffer->buf[i + 1] == '\n')) {
+   if (i != j)
+   buffer->buf[j] = buffer->buf[i];
+   j++;
+   }
+   strbuf_setlen(buffer, j);
+}
+
 /*
  * Create a detached signature for the contents of "buffer" and append
  * it after "signature"; "buffer" and "signature" can be the same
@@ -155,7 +169,7 @@ int sign_buffer(struct strbuf *buffer, struct strbuf 
*signature, const char *sig
 {
struct child_process gpg = CHILD_PROCESS_INIT;
int ret;
-   size_t i, j, bottom;
+   size_t bottom;
struct strbuf gpg_status = STRBUF_INIT;
 
argv_array_pushl(,
@@ -180,14 +194,7 @@ int sign_buffer(struct strbuf *buffer, struct strbuf 
*signature, const char *sig
if (ret)
return error(_("gpg failed to sign the data"));
 
-   /* Strip CR from the line endings, in case we are on Windows. */
-   for (i = j = bottom; i < signature->len; i++)
-   if (signature->buf[i] != '\r') {
-   if (i != j)
-   signature->buf[j] = signature->buf[i];
-   j++;
-   }
-   strbuf_setlen(signature, j);
+   strip_cr(signature, bottom);
 
return 0;
 }
@@ -230,6 +237,8 @@ int verify_signed_buffer(const char *payload, size_t 
payload_size,
sigchain_push(SIGPIPE, SIG_IGN);
ret = pipe_command(, payload, payload_size,
   gpg_status, 0, gpg_output, 0);
+   strip_cr(gpg_status, 0);
+   strip_cr(gpg_output, 0);
sigchain_pop(SIGPIPE);
 
delete_tempfile();
-- 
2.14.2.windows.3

Re: [PATCH] mru: Replace mru.[ch] with list.h implementation

[Gargi Sharma]

On Sun, Nov 12, 2017 at 9:54 AM, Jeff King  wrote:
> On Sat, Nov 11, 2017 at 01:06:46PM -0500, Gargi Sharma wrote:
>
>> Replace custom allocation in mru.[ch] with generic calls
>> to list.h API.
>>
>> Signed-off-by: Gargi Sharma 
>
> Thanks, and welcome to the git list. :)
>
> This looks like a good start on the topic, but I have a few comments.
>
> It's a good idea to explain in the commit message not just what we're
> doing, but why we want to do it, to help later readers of "git log". I
> know that you picked this up from the discussion in the thread at:
>
>   https://public-inbox.org/git/xmqq8tgz13x7@gitster.mtv.corp.google.com/
>
> so it might be a good idea to summarize the ideas there (and add your
> own thoughts, of course).
>
>> ---
>>  builtin/pack-objects.c | 14 --
>>  cache.h|  9 +
>>  mru.c  | 27 ---
>>  mru.h  | 40 
>>  packfile.c | 28 +++-
>>  5 files changed, 32 insertions(+), 86 deletions(-)
>>  delete mode 100644 mru.c
>>  delete mode 100644 mru.h
>
> After the "---" line, you can put any information that people on the
> list might want to know but that doesn't need to go into the commit
> message. The big thing the maintainer would want to know here is that
> your patch is prepared on top of the ot/mru-on-list topic, so he knows
> where to apply it.
>
> The diffstat is certainly encouraging so far. :)
>
>> @@ -1012,9 +1012,9 @@ static int want_object_in_pack(const unsigned char 
>> *sha1,
>>   return want;
>>   }
>>
>> - list_for_each(pos, _git_mru.list) {
>> - struct mru *entry = list_entry(pos, struct mru, list);
>> - struct packed_git *p = entry->item;
>> + list_for_each(pos, _git_mru) {
>> + struct packed_git *p = list_entry(pos, struct packed_git, mru);
>> + struct list_head *entry = &(p->mru);
>>   off_t offset;
>>
>>   if (p == *found_pack)
>
> I think "entry" here is going to be the same as "pos". That said, I
> think it's only use is in bumping us to the front of the mru list later:
>
>> @@ -1030,8 +1030,10 @@ static int want_object_in_pack(const unsigned char 
>> *sha1,
>>   *found_pack = p;
>>   }
>>   want = want_found_object(exclude, p);
>> - if (!exclude && want > 0)
>> - mru_mark(_git_mru, entry);
>> + if (!exclude && want > 0) {
>> + list_del(entry);
>> + list_add(entry, _git_mru);
>> + }
>
> And I think this might be more obvious if we drop "entry" entirely and
> just do:
>
>   list_del(>mru);
>   list_add(>mru, _git_mru);
>
> It might merit a comment like "/* bump to the front of the mru list */"
> or similar to make it clear what's going on (or even adding a
> list_move_to_front() helper).

I will add a helper to list.h, for doing this :)
>
>> @@ -1566,6 +1566,7 @@ struct pack_window {
>>
>>  extern struct packed_git {
>>   struct packed_git *next;
>> + struct list_head mru;
>>   struct pack_window *windows;
>>   off_t pack_size;
>>   const void *index_data;
>
> Sort of a side note, but seeing these two list pointers together makes
> me wonder what we should do with the list created by the "next" pointer.
> It seems like there are three options:
>
>   1. Convert it to "struct list_head", too, for consistency.
>
>   2. Leave it as-is. We never delete from the list nor do any fancy
>  manipulation, so it doesn't benefit from the reusable code.
>
>   3. I wonder if we could drop it entirely, and just keep a single list
>  of packs, ordered by mru. I'm not sure if anybody actually cares
>  about accessing them in the "original" order. That order is
>  reverse-chronological (by prepare_packed_git()), but I think that
>  was mostly out of a sense that recent packs would be accessed more
>  than older ones (but having a real mru strategy replaces that
>  anyway).
>
> What do you think?
I think in the long run, it'll be easier if there is only a single
list of packs given
that no one needs to access the list in order.

If we go down road 1/3, would it be better if I sent an entirely
different patch or
a patch series with patch 1 as removing mru[.ch] and patch 2 as removing
next pointer from the struct?

>
>> diff --git a/mru.c b/mru.c
>> deleted file mode 100644
>> index 8f3f34c..000
>
> Yay, this hunk (and the one for mru.h) is satisfying.
>
>> @@ -40,7 +40,7 @@ static unsigned int pack_max_fds;
>>  static size_t peak_pack_mapped;
>>  static size_t pack_mapped;
>>  struct packed_git *packed_git;
>> -struct mru packed_git_mru = {{_git_mru.list, _git_mru.list}};
>> +LIST_HEAD(packed_git_mru);
>
> Much nicer.
>
>> 

[PATCH] config: added --expiry-date type support

[Haaris]

---
 builtin/config.c   | 11 ++-
 config.c   |  9 +
 config.h   |  1 +
 t/t1300-repo-config.sh | 25 +
 4 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/builtin/config.c b/builtin/config.c
index d13daeeb55927..41cd9f5ca7cde 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,12 @@ 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 = malloc(sizeof(*t));
+   if(git_config_expiry_date(, key_, value_) < 0)
+   return -1;
+   strbuf_addf(buf, "%"PRItime, *t);
+   free((timestamp_t *)t);
} else if (value_) {
strbuf_addstr(buf, value_);
} else {
@@ -273,12 +281,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/config.c b/config.c
index 903abf9533b18..caa2fd5fb6915 100644
--- a/config.c
+++ b/config.c
@@ -990,6 +990,15 @@ 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))
+   die(_("failed to parse date_string in: '%s'"), value);
+   return 0;
+}
+
 static int git_default_core_config(const char *var, const char *value)
 {
/* This needs a better name */
diff --git a/config.h b/config.h
index a49d264416225..2d127d9d23c90 100644
--- a/config.h
+++ b/config.h
@@ -58,6 +58,7 @@ extern int git_config_bool_or_int(const char *, const char *, 
int *);
 extern int git_config_bool(const char *, const char *);
 extern int git_config_string(const char **, const char *, const char *);
 extern int git_config_pathname(const char **, const char *, const char *);
+extern int git_config_expiry_date(timestamp_t **, const char *, const char *);
 extern int git_config_set_in_file_gently(const char *, const char *, const 
char *);
 extern void git_config_set_in_file(const char *, const char *, const char *);
 extern int git_config_set_gently(const char *, const char *);
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 364a537000bbb..59a35be89e511 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -901,6 +901,31 @@ test_expect_success 'get --path barfs on boolean variable' 
'
test_must_fail git config --get --path path.bool
 '
 
+test_expect_success 'get --expiry-date' '
+   cat >.git/config <<-\EOF &&
+   [date]
+   valid1 = "Fri Jun 4 15:46:55 2010"
+   valid2 = "2017/11/11 11:11:11PM"
+   valid3 = "2017/11/10 09:08:07 PM"
+   valid4 = "never"
+   invalid1 = "abc"
+   EOF
+   cat >expect <<-\EOF &&
+   1275666415
+   1510441871
+   1510348087
+   0
+   EOF
+   {
+   git config --expiry-date date.valid1 &&
+   git config --expiry-date date.valid2 &&
+   git config --expiry-date date.valid3 &&
+   git config --expiry-date date.valid4
+   } >actual &&
+   test_cmp expect actual &&
+   test_must_fail git config --expiry-date date.invalid1
+'
+
 cat > expect 

Re: "git bisect" takes exactly one bad commit and one or more good?

[Kaartic Sivaraam]

On Sat, 2017-11-11 at 10:27 -0500, Robert P. J. Day wrote:
> 
>   i realize that one of each commit is the simplest use case, but the
> scenario that occurred to me is a bunch of branches being merged and,
> suddenly, you have a bug, and you're not sure where it came from so
> you identify a number of good commits, one per merged branch, and go
> from there.
> 
> 

Just thinking out loud, couldn't you give the one commit that was the
tip of the branch, to which you merged the branches, before you merged
in the branches as the good commit ?


-- 
Kaartic

Re: NO_MSGFMT

[Jeff King]

On Sun, Aug 13, 2017 at 12:58:13AM -0400, Jeff King wrote:

> On Sat, Aug 12, 2017 at 03:44:17PM +0200, Dominik Mahrer (Teddy) wrote:
> 
> > Hi all
> > 
> > I'm compiling git from source code on a mashine without msgfmt. This leads
> > to compile errors. To be able to compile git I created a patch that at least
> > works for me:
> 
> Try:
> 
>   make NO_MSGFMT=Nope NO_GETTEXT=Nope
> 
> This also works:
> 
>   make NO_GETTEXT=Nope NO_TCLTK=Nope
> 
> The flags to avoid gettext/msgfmt are sadly different between git itself
> and git-gui/gitk, which we include as a subproject. It would be a useful
> patch to harmonize though (probably by accepting both in all places for
> compatibility).

I saw somebody else today run into problems about gettext, so I thought
I'd revisit this and write that patch. It turns out the situation is
slightly different than I thought. So no patch, but I wanted to report
here what I found.

It's true that the option is called NO_GETTEXT in git.git, but NO_MSGFMT
in the tcl programs we pull in. So I figured to start with a patch that
turns on NO_MSGFMT automatically when NO_GETTEXT is set. But it's
not necessary.

The gitk and git-gui tests actually check that msgfmt is available.
If it isn't, they automatically fall back to using a pure-tcl
implementation. So there's generally no need to set NO_MSGFMT at
all.

But that fallback is implemented using tcl. So if you _also_ don't have
tcl installed (and I don't), you get quite a confusing output from make:

  $ make -j1 
SUBDIR git-gui
MSGFMT po/pt_pt.msg Makefile:252: recipe for target 'po/pt_pt.msg' failed
make[1]: *** [po/pt_pt.msg] Error 127

If you run with V=1, you can see that it's not running msgfmt at all,
but:

  tclsh po/po2msg.sh --statistics --tcl -l pt_pt -d po/ po/pt_pt.po 

So my takeaways are:

  1. You should never need to set NO_MSGFMT; it falls back
 automatically.

  2. If you don't have gettext, you should set NO_GETTEXT to tell the
 rest of git not to use it.

  3. If you see msgfmt errors even after NO_GETTEXT, try NO_TCLTK.

-Peff

[no subject]

[hsed]

subscribe git

[PATCH] link_alt_odb_entries: make empty input a noop

[Jeff King]

On Sat, Nov 11, 2017 at 11:13:21AM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > So totally orthogonal to your bug, I wonder if we ought to be doing:
> >
> > diff --git a/sha1_file.c b/sha1_file.c
> > index 057262d46e..0b76233aa7 100644
> > --- a/sha1_file.c
> > +++ b/sha1_file.c
> > @@ -530,11 +530,11 @@ void prepare_alt_odb(void)
> > if (alt_odb_tail)
> > return;
> >  
> > -   alt = getenv(ALTERNATE_DB_ENVIRONMENT);
> > -   if (!alt) alt = "";
> > -
> > alt_odb_tail = _odb_list;
> > -   link_alt_odb_entries(alt, strlen(alt), PATH_SEP, NULL, 0);
> > +
> > +   alt = getenv(ALTERNATE_DB_ENVIRONMENT);
> > +   if (alt)
> > +   link_alt_odb_entries(alt, strlen(alt), PATH_SEP, NULL, 0);
> >  
> > read_info_alternates(get_object_directory(), 0);
> >  }
> >
> > to avoid hitting link_alt_odb_entries() at all when there are no
> > entries.
> 
> Sounds sane.

Here it is as a real patch. I actually bumped the check into the
function itself, since it keeps the logic all in one place. And as a
bonus, we save work if you truly have an empty environment variable or
info/alternates file, though I don't expect those are very common. :)

I also rebased on top of dc732bd5cb (read_info_alternates: read contents
into strbuf, 2017-09-19), which had a trivial textual conflict.

This should make Joey's immediate pain go away, though only by papering
it over. I tend to agree that we shouldn't be looking at $PWD at all
here.

-- >8 --
Subject: [PATCH] link_alt_odb_entries: make empty input a noop

If an empty string is passed to link_alt_odb_entries(), our
loop finds no entries and we link nothing. But we still do
some preparatory work to normalize the object directory
path, even though we'll never look at the result. This
triggers in basically every git process, since we feed the
usually-empty ALTERNATE_DB_ENVIRONMENT to the function.

Let's detect early that there's nothing to do and return.
While we're at it, let's treat NULL the same as an empty
string as a favor to our callers. That saves
prepare_alt_odb() from having to cover this case.

Signed-off-by: Jeff King 
---
 sha1_file.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sha1_file.c b/sha1_file.c
index d708981376..8a7c6b7eba 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -404,6 +404,9 @@ static void link_alt_odb_entries(const char *alt, int sep,
struct strbuf objdirbuf = STRBUF_INIT;
struct strbuf entry = STRBUF_INIT;
 
+   if (!alt || !*alt)
+   return;
+
if (depth > 5) {
error("%s: ignoring alternate object stores, nesting too deep.",
relative_base);
@@ -604,7 +607,6 @@ void prepare_alt_odb(void)
return;
 
alt = getenv(ALTERNATE_DB_ENVIRONMENT);
-   if (!alt) alt = "";
 
alt_odb_tail = _odb_list;
link_alt_odb_entries(alt, PATH_SEP, NULL, 0);
-- 
2.15.0.413.g6cc52d366b

Re: [PATCH] mru: Replace mru.[ch] with list.h implementation

[Jeff King]

On Sun, Nov 12, 2017 at 09:54:35AM +, Jeff King wrote:

> > @@ -1566,6 +1566,7 @@ struct pack_window {
> >  
> >  extern struct packed_git {
> > struct packed_git *next;
> > +   struct list_head mru;
> > struct pack_window *windows;
> > off_t pack_size;
> > const void *index_data;
> 
> Sort of a side note, but seeing these two list pointers together makes
> me wonder what we should do with the list created by the "next" pointer.
> It seems like there are three options:
> 
>   1. Convert it to "struct list_head", too, for consistency.
> 
>   2. Leave it as-is. We never delete from the list nor do any fancy
>  manipulation, so it doesn't benefit from the reusable code.
> 
>   3. I wonder if we could drop it entirely, and just keep a single list
>  of packs, ordered by mru. I'm not sure if anybody actually cares
>  about accessing them in the "original" order. That order is
>  reverse-chronological (by prepare_packed_git()), but I think that
>  was mostly out of a sense that recent packs would be accessed more
>  than older ones (but having a real mru strategy replaces that
>  anyway).
> 
> What do you think?

Thinking on this a bit more, even if we want to go down any road except
(2), it probably ought to come as a separate patch on top anyway. The
changes you're making here are quite obviously a noop for visible
behavior.

But dropping the "next" pointer (and the matching "don't clear the list"
I mentioned later) would potentially mean examining the packs in a
slightly different order. I _think_ that's fine, but it's possible there
could be a subtle fallout. So it's better to keep it separate from the
more pure refactoring in your patch.

-Peff

Re: should "git bisect" support "git bisect next?"

[Junio C Hamano]

"Robert P. J. Day"  writes:

>> This reminds me; is there a way to suppress it because I'm about to
>> give a large set of good and bit commits (perhaps because I'm
>   
>> replaying part of a git biset log, minus one or two lines that are
>> suspected of being bogus thanks to flaky reproduction), and so
>> there's no point having git bisect figure the "next" commit to try
>> until I'm done giving it a list of good/bad commits?
>
>   i'm sure i'll regret asking this, but (assuming "bit" should read
> "bad") is this suggesting one can hand bisect more than one bad
> commit? i thought we just went through that discussion where there
> could be only one bad commit but multiple good commits. clarification?

The documentation you have been futzing with is about the fact that
the initial set of known to be good/bad commits that "git bisect
start  ..." take can have one bad and zero or more good.

What is being discussed in this thread is different (and I tried to
clarify the fact by saying "what bad and good commits in what
sequence").  

Ted is talking about replaying the series of "git bisect (good|bad)
$a_single_commit" that are recorded during a bisection session and
can be read via "git bisect log".  When Ted says "good commits"
and/or "bad commits", he is not talking about giving all of them to
a single invocation of "git bisect" command.  The replay session
will take one commit at a time (which is what "git bisect replay"
does) and feed it to either "git bisect good" or "git bisect bad".

Does it make sense?

By the way, I do not think there is anything to regret in asking
what you do not understand.  Showing how much you know (and you
don't) will allow others who communicate with you to calibrate their
expectations, which eases later discussions; it is a good thing.

Re: [PATCH] mru: Replace mru.[ch] with list.h implementation

[Jeff King]

On Sat, Nov 11, 2017 at 01:06:46PM -0500, Gargi Sharma wrote:

> Replace custom allocation in mru.[ch] with generic calls
> to list.h API.
> 
> Signed-off-by: Gargi Sharma 

Thanks, and welcome to the git list. :)

This looks like a good start on the topic, but I have a few comments.

It's a good idea to explain in the commit message not just what we're
doing, but why we want to do it, to help later readers of "git log". I
know that you picked this up from the discussion in the thread at:

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

so it might be a good idea to summarize the ideas there (and add your
own thoughts, of course).

> ---
>  builtin/pack-objects.c | 14 --
>  cache.h|  9 +
>  mru.c  | 27 ---
>  mru.h  | 40 
>  packfile.c | 28 +++-
>  5 files changed, 32 insertions(+), 86 deletions(-)
>  delete mode 100644 mru.c
>  delete mode 100644 mru.h

After the "---" line, you can put any information that people on the
list might want to know but that doesn't need to go into the commit
message. The big thing the maintainer would want to know here is that
your patch is prepared on top of the ot/mru-on-list topic, so he knows
where to apply it.

The diffstat is certainly encouraging so far. :)

> @@ -1012,9 +1012,9 @@ static int want_object_in_pack(const unsigned char 
> *sha1,
>   return want;
>   }
>  
> - list_for_each(pos, _git_mru.list) {
> - struct mru *entry = list_entry(pos, struct mru, list);
> - struct packed_git *p = entry->item;
> + list_for_each(pos, _git_mru) {
> + struct packed_git *p = list_entry(pos, struct packed_git, mru);
> + struct list_head *entry = &(p->mru);
>   off_t offset;
>  
>   if (p == *found_pack)

I think "entry" here is going to be the same as "pos". That said, I
think it's only use is in bumping us to the front of the mru list later:

> @@ -1030,8 +1030,10 @@ static int want_object_in_pack(const unsigned char 
> *sha1,
>   *found_pack = p;
>   }
>   want = want_found_object(exclude, p);
> - if (!exclude && want > 0)
> - mru_mark(_git_mru, entry);
> + if (!exclude && want > 0) {
> + list_del(entry);
> + list_add(entry, _git_mru);
> + }

And I think this might be more obvious if we drop "entry" entirely and
just do:

  list_del(>mru);
  list_add(>mru, _git_mru);

It might merit a comment like "/* bump to the front of the mru list */"
or similar to make it clear what's going on (or even adding a
list_move_to_front() helper).

> @@ -1566,6 +1566,7 @@ struct pack_window {
>  
>  extern struct packed_git {
>   struct packed_git *next;
> + struct list_head mru;
>   struct pack_window *windows;
>   off_t pack_size;
>   const void *index_data;

Sort of a side note, but seeing these two list pointers together makes
me wonder what we should do with the list created by the "next" pointer.
It seems like there are three options:

  1. Convert it to "struct list_head", too, for consistency.

  2. Leave it as-is. We never delete from the list nor do any fancy
 manipulation, so it doesn't benefit from the reusable code.

  3. I wonder if we could drop it entirely, and just keep a single list
 of packs, ordered by mru. I'm not sure if anybody actually cares
 about accessing them in the "original" order. That order is
 reverse-chronological (by prepare_packed_git()), but I think that
 was mostly out of a sense that recent packs would be accessed more
 than older ones (but having a real mru strategy replaces that
 anyway).

What do you think?

> diff --git a/mru.c b/mru.c
> deleted file mode 100644
> index 8f3f34c..000

Yay, this hunk (and the one for mru.h) is satisfying.

> @@ -40,7 +40,7 @@ static unsigned int pack_max_fds;
>  static size_t peak_pack_mapped;
>  static size_t pack_mapped;
>  struct packed_git *packed_git;
> -struct mru packed_git_mru = {{_git_mru.list, _git_mru.list}};
> +LIST_HEAD(packed_git_mru);

Much nicer.

> @@ -859,9 +859,18 @@ static void prepare_packed_git_mru(void)
>  {
>   struct packed_git *p;
>  
> - mru_clear(_git_mru);
> - for (p = packed_git; p; p = p->next)
> - mru_append(_git_mru, p);
> + struct list_head *pos;
> + struct list_head *tmp;
> + list_for_each_safe(pos, tmp, _git_mru)
> + list_del_init(pos);

This matches the original code, which did the clear/re-create, resetting
the mru to the "original" pack order. But I do wonder if that's actually
necessary. Could we skip that and just add any new packs to the list?

That goes 

more pedantry ... what means a file "known to Git"?

[Robert P. J. Day]

  apologies for more excruciating nitpickery, but i ask since it seems
that phrase means slightly different things depending on where you
read it.

  first, i assume that there are only two categories:

  1) files known to Git
  2) files unknown to Git

and that there is no fuzzy, grey area middle ground, yes?

  now, in "man git-clean", one reads (near the top):

Cleans the working tree by recursively removing files that are
not under version control, starting from the current directory.

Normally, only files unknown to Git are removed, but if the -x
  ^
option is specified, ignored files are also removed.

the way that's worded suggests that ignored files are "known" to Git,
yes? that is, if, by default, "git clean" removes only files "unknown"
to Git, and "-x" extends that to ignored files, the conclusion is that
ignored files are *known* to Git.

  if, however, you check out "man git-rm", you read:

The  list given to the command can be exact pathnames,
file glob patterns, or leading directory names. The command
removes only the paths that are known to Git. Giving the name

of a file that you have not told Git about does not remove that file.

so "git rm" removes only files "known to Git", but from the above
regarding how "git clean" sees this, that should include ignored
files, which of course it doesn't.

  given that this phrase occurs in a number of places:

$ grep -ir "known to git" *
builtin/difftool.c: /* The symlink is unknown to Git so read from 
the filesystem */
dir.c:  error("pathspec '%s' did not match any file(s) known to git.",
Documentation/git-rm.txt:removes only the paths that are known to Git.  Giving 
the name of
Documentation/git-commit.txt:   be known to Git);
Documentation/user-manual.txt:error: pathspec 
'261dfac35cb99d380eb966e102c1197139f7fa24' did not match any file(s) known to 
git.
Documentation/gitattributes.txt:Notice all types of potential 
whitespace errors known to Git.
Documentation/git-clean.txt:Normally, only files unknown to Git are removed, 
but if the `-x`
Documentation/RelNotes/1.8.2.1.txt: * The code to keep track of what directory 
names are known to Git on
Documentation/RelNotes/1.8.1.6.txt: * The code to keep track of what directory 
names are known to Git on
Documentation/RelNotes/2.9.0.txt:   known to Git.  They have been taught to do 
the normalization.
Documentation/RelNotes/2.8.4.txt:   known to Git.  They have been taught to do 
the normalization.
Documentation/RelNotes/1.8.3.txt: * The code to keep track of what directory 
names are known to Git on
t/t3005-ls-files-relative.sh:   echo "error: pathspec $sq$f$sq 
did not match any file(s) known to git."
t/t3005-ls-files-relative.sh:   echo "error: pathspec $sq$f$sq 
did not match any file(s) known to git."
$

it might be useful to define precisely what it means. or is it assumed
to be context dependent?

rday

-- 


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

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

[PATCH v4] bisect: mention "view" as an alternative to "visualize"

[Robert P. J. Day]

Tweak a small number of files to mention "view" as an alternative to
"visualize".

Signed-off-by: Robert P. J. Day 

---

  ok, let's see if this one is getting close ...

 Documentation/git-bisect.txt | 13 ++---
 builtin/bisect--helper.c |  2 +-
 git-bisect.sh|  4 ++--
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index 6c42abf07..4a1417bdc 100644
--- a/Documentation/git-bisect.txt
+++ b/Documentation/git-bisect.txt
@@ -23,7 +23,7 @@ on the subcommand:
  git bisect terms [--term-good | --term-bad]
  git bisect skip [(|)...]
  git bisect reset []
- git bisect visualize
+ git bisect (visualize|view)
  git bisect replay 
  git bisect log
  git bisect run ...
@@ -193,24 +193,23 @@ git bisect start --term-new fixed --term-old broken
 Then, use `git bisect ` and `git bisect ` instead
 of `git bisect good` and `git bisect bad` to mark commits.

-Bisect visualize
-
+Bisect visualize/view
+~

 To see the currently remaining suspects in 'gitk', issue the following
-command during the bisection process:
+command during the bisection process (the subcommand `view` can be used
+as an alternative to `visualize`):

 
 $ git bisect visualize
 

-`view` may also be used as a synonym for `visualize`.
-
 If the `DISPLAY` environment variable is not set, 'git log' is used
 instead.  You can also give command-line options such as `-p` and
 `--stat`.

 
-$ git bisect view --stat
+$ git bisect visualize --stat
 

 Bisect log and bisect replay
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 35d2105f9..4b5fadcbe 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -46,7 +46,7 @@ static int check_term_format(const char *term, const char 
*orig_term)
return error(_("'%s' is not a valid term"), term);

if (one_of(term, "help", "start", "skip", "next", "reset",
-   "visualize", "replay", "log", "run", "terms", NULL))
+   "visualize", "view", "replay", "log", "run", "terms", 
NULL))
return error(_("can't use the builtin command '%s' as a term"), 
term);

/*
diff --git a/git-bisect.sh b/git-bisect.sh
index 0138a8860..a82256e34 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -1,6 +1,6 @@
 #!/bin/sh

-USAGE='[help|start|bad|good|new|old|terms|skip|next|reset|visualize|replay|log|run]'
+USAGE='[help|start|bad|good|new|old|terms|skip|next|reset|visualize|view|replay|log|run]'
 LONG_USAGE='git bisect help
print this long help message.
 git bisect start [--term-{old,good}= --term-{new,bad}=]
@@ -20,7 +20,7 @@ git bisect next
find next bisection to test and check it out.
 git bisect reset []
finish bisection search and go back to commit.
-git bisect visualize
+git bisect (visualize|view)
show bisect status in gitk.
 git bisect replay 
replay bisection log.


-- 


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

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

Re: [PATCH v3] bisect: mention "view" as an alternative to "visualize"

[Robert P. J. Day]

On Sun, 12 Nov 2017, Junio C Hamano wrote:

> "Robert P. J. Day"  writes:
>
> >  To see the currently remaining suspects in 'gitk', issue the following
> > -command during the bisection process:
> > +command during the bisection process (the subcommand `view` can, in all
> > +cases, be used as an alternative to `visualize`):
>
> I'd drop ", in all cases," if I were writing this.
>
> If it were very common that some "synonyms" are only usable in
> certain limited cases, singling this out and explicitly saying that
> "'view', unlike many other 'synonyms', is truly a synonym to
> visualize in all cases" would make sense and would help readers, but
> I do not think that is the case.  An alternative by definition
> should be usable "in all cases", so I do not think the phrase helps
> the readers at all.
>
> > diff --git a/contrib/completion/git-completion.bash 
> > b/contrib/completion/git-completion.bash
> > index fdd984d34..52f68c922 100644
> > --- a/contrib/completion/git-completion.bash
> > +++ b/contrib/completion/git-completion.bash
> > @@ -1162,7 +1162,7 @@ _git_bisect ()
> >  {
> > __git_has_doubledash && return
> >
> > -   local subcommands="start bad good skip reset visualize replay log run"
> > +   local subcommands="start bad good skip reset visualize view replay log 
> > run"
>
> This change makes the end user experience a lot worse, I am afraid.
>
> People used to be able to say "bisect vi" and I'd imagine that
> many are used to type exactly that.  Now they get two choices and
> have to say 's' (or 'e') before hitting another .

  good points, i'll re-roll and re-submit.

rday

-- 


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

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

Re: should "git bisect" support "git bisect next?"

[Robert P. J. Day]

On Sat, 11 Nov 2017, Theodore Ts'o wrote:

> On Sat, Nov 11, 2017 at 11:38:23PM +0900, Junio C Hamano wrote:
> >
> > Thanks for saving me time to explain why 'next' is still a very
> > important command but the end users do not actually need to be
> > strongly aware of it, because most commands automatically invokes
> > it as their final step due to the importance of what it does ;-)
>
> This reminds me; is there a way to suppress it because I'm about to
> give a large set of good and bit commits (perhaps because I'm
  
> replaying part of a git biset log, minus one or two lines that are
> suspected of being bogus thanks to flaky reproduction), and so
> there's no point having git bisect figure the "next" commit to try
> until I'm done giving it a list of good/bad commits?

  i'm sure i'll regret asking this, but (assuming "bit" should read
"bad") is this suggesting one can hand bisect more than one bad
commit? i thought we just went through that discussion where there
could be only one bad commit but multiple good commits. clarification?

rday

-- 


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

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