What's cooking in git.git (Nov 2017, #01; Wed, 1)

[Junio C Hamano]

Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.  The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.

Git 2.15 final was tagged.  I may not have fully caught up with the
list traffic and there may be topics that should have been scooped
up but lost in the noise, but I think the tree is in a better shape
than yesterday ;-)  We'll see if there is any brown paper bag bugs
by waiting til the end of the week.

And then we'll start the new cycle by rewinding the top of 'next'
and also marking topics that are marked as "will cook in 'next'" as
"will merge to 'master'" around the weekend.  The first to graduate
will be the ex/deprecate-empty-pathspec-as-match-all topic.

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[New Topics]

* ad/5580-unc-tests-on-cygwin (2017-11-01) 1 commit
 - t5580: add Cygwin support

 UNC paths are also relevant in Cygwin builds and they are now
 tested just like Mingw builds.

 Will merge to 'next'.


* ao/merge-verbosity-getenv-just-once (2017-11-01) 1 commit
 - merge-recursive: check GIT_MERGE_VERBOSITY only once

 Code cleanup.

 Will merge to 'next'.


* bp/read-index-from-skip-verification (2017-11-01) 1 commit
 - read_index_from(): speed index loading by skiping verification of the entry 
order

 Drop (perhaps overly cautious) sanity check before using the index
 read from the filesystem at runtime.

 Will merge to 'next'.


* cn/diff-indent-no-longer-is-experimental (2017-11-01) 2 commits
 - SQUASH???
 - diff: --indent-heuristic is no longer experimental

 Doc update.

 Will merge to 'next' after squashing the fixup.


* jk/rebase-i-exec-gitdir-fix (2017-11-01) 1 commit
 - sequencer: pass absolute GIT_DIR to exec commands

 A recent regression in "git rebase -i" that broke execution of git
 commands from subdirectories via "exec" insn has been fixed.

 Ack from Dscho would be nice.


* mh/test-local-canary (2017-10-31) 1 commit
 - t: check whether the shell supports the "local" keyword

 We try to see if somebody runs our test suite with a shell that
 does not support "local" like bash/dash does.

 Will merge to 'next'.


* rs/hex-to-bytes-cleanup (2017-11-01) 3 commits
 - sha1_file: use hex_to_bytes()
 - http-push: use hex_to_bytes()
 - notes: move hex_to_bytes() to hex.c and export it

 Code cleanup.

 Will merge to 'next'.


* rs/sequencer-rewrite-file-cleanup (2017-11-01) 2 commits
 - sequencer: use O_TRUNC to truncate files
 - sequencer: factor out rewrite_file()

 Code cleanup.

 Will merge to 'next'.


* sb/describe-blob (2017-11-01) 7 commits
 - t6120: fix typo in test name
 - builtin/describe.c: describe a blob
 - builtin/describe.c: factor out describe_commit
 - builtin/describe.c: print debug statements earlier
 - builtin/describe.c: rename `oid` to avoid variable shadowing
 - revision.h: introduce blob/tree walking in order of the commits
 - list-objects.c: factor out traverse_trees_and_blobs

 "git describe" was taught to dig trees deeper to find a
 : that refers to a given blob object.


* tb/add-renormalize (2017-10-31) 1 commit
 - add: introduce "--renormalize"

 "git add --renormalize ." is a new and safer way to record the fact
 that you are correcting the end-of-line convention and other
 "convert_to_git()" glitches in the in-repository data.


* ab/mediawiki-name-truncation (2017-11-01) 1 commit
 - remote-mediawiki: limit filenames to legal

 The remote-helper for talking to MediaWiki has been updated to
 truncate an overlong pagename so that ".mw" suffix can still be
 added.

 Will merge to 'next'.


* ab/mediawiki-namespace (2017-11-01) 7 commits
 - remote-mediawiki: show progress while fetching namespaces
 - remote-mediawiki: process namespaces in order
 - remote-mediawiki: support fetching from (Main) namespace
 - remote-mediawiki: skip virtual namespaces
 - remote-mediawiki: show known namespace choices on failure
 - remote-mediawiki: allow fetching namespaces with spaces
 - remote-mediawiki: add namespace support

 The remote-helper for talking to MediaWiki has been updated to
 work with mediawiki namespaces.

 Will merge to 'next'.


* js/mingw-full-version-in-resources (2017-11-01) 1 commit
 - mingw: include the full version information in the resources

 MinGW updates.

 Will merge to 'next'.

* js/mingw-redirection (2017-11-01) 3 commits
 - mingw: document the experimental standard handle redirection
 - mingw: special-case GIT_REDIRECT_STDERR=2>&1
 - mingw: add experimental feature to redirect standard handles

 MinGW updates.


* js/wincred-empty-cred (2017-11-01) 2 commits
 - wincred: handle empty username/password correctly
 - t0302: check helper can handle empty credentials

 MinGW updates.

Re: [PATCHv2 0/7] git describe blob

[Junio C Hamano]

Stefan Beller  writes:

> Occasionally a user is given an object hash from a blob as an error message
> or other output (e.g. [1]).
>
> It would be useful to get a further description of such a blob, such as
> the (commit, path) tuple where this blob was introduced.
>
> This implements the answer in builtin/describe,
> however the heuristics are weak. See patch 6 for details.

Overall this was a relatively pleasant read.  Will queue.

Thanks.

Re: [PATCH 3/3] mingw: document the experimental standard handle redirection

[Junio C Hamano]

Johannes Schindelin  writes:

>> > +Two special values are supported: `off` will simply close the
>> > +corresponding standard handle, and if `GIT_REDIRECT_STDERR` is
>> > +`2>&1`, standard error will be redirected to the same handle as
>> > +standard output.
>> 
>> Consistent with the Unixy special-case for '2>&1', I wonder if the
>> 'off' case would be more intuitively stated as '>/dev/null' or just
>> '/dev/null'...
>
> I feel this is the wrong way round. `>/dev/null` may sound very intuitive
> to you, but this feature is Windows only. Guess three times how intuitive
> it sounds to Windows developers to write `>/dev/null` if you want to
> suppress output...

It would be just as intuitive to write '2>&1' for dup-redirection,
so I tend to agree with both of you in that perhaps '2>&1' may have
to become less Unix-y (or more Windows-y) to make these special
cases more consistent.  Perhaps "dup-to-stdout" or even just "stdout".

Side note: if we really wanted to go in the other direction
Eric suggests, "off" probably should be spelled as ">&-" ;-)

By the way, the description talks about "special values", but it
leaves it completely unclear what their normal values mean.  Are
they filenames, or integers that denote file descriptors, or
something else?  To those who read, wrote, or reviewed the code in
these patches, the answer is obvious (and I do *not* want you to
give your answer to *me* in your response for that reason).  But
we'd want to give the answer to future readers by clarifying this
documentation.

Re: [PATCHv2 6/7] builtin/describe.c: describe a blob

[Junio C Hamano]

Stefan Beller  writes:

> +If the given object refers to a blob, it will be described
> +as `:`, such that the blob can be found
> +at `` in the ``. Note, that the commit is likely

Does the code describe a9dbc3f12c as v2.15.0:GIT-VERSION-GEN, or
would it always be :?

> +not the commit that introduced the blob, but the one that was found
> +first; to find the commit that introduced the blob, you need to find

Because we do not want to descend into the same tree object we saw
earlier, this "we show the name we happened to find first without
attempting to refine it for a better name" is a rather fundamental
limitation of this approach.  Hopefully we can later improve it with
more thought, but for now it is better than nothing (and much better
than "git log --raw | grep").

> +the commit that last touched the path, e.g.
> +`git log  -- `.
> +As blobs do not point at the commits they are contained in,
> +describing blobs is slow as we have to walk the whole graph.

Is it true that we walk the "whole graph", or do we stop immediately
we find a path to the blob?  The former makes it sound like
completely useless.

> -#define SEEN (1u << 0)

Interesting.  Now we include revision.h this becomes redundant.

> +static void describe_blob(struct object_id oid, struct strbuf *dst)
> +{
> + struct rev_info revs;
> + struct argv_array args = ARGV_ARRAY_INIT;
> + struct process_commit_data pcd = { null_oid, oid, dst};
> +
> + argv_array_pushl(, "internal: The first arg is not parsed",
> + "--all", "--reflog", /* as many starting points as possible */

Interesting.  

Do we also search in the reflog in the normal "describe" operation?
If not, perhaps we should?  I wonder what's the performance
implications would be.

That tangent aside, as "describe blob" is most likely a "what
reaches and is holding onto this blob?" query, finding something
that can only be reached from a reflog entry would make it more
helpful than without the option.

> + /* NEEDSWORK: --all is incompatible with worktrees for now: */

What's that last colon about?

> + "--single-worktree",
> + "--objects",
> + "--in-commit-order",
> + NULL);
> +
> + init_revisions(, NULL);
> + if (setup_revisions(args.argc, args.argv, , NULL) > 1)
> + BUG("setup_revisions could not handle all args?");
> +
> + if (prepare_revision_walk())
> + die("revision walk setup failed");
> +
> + traverse_commit_list(, process_commit, process_object, );
> + reset_revision_walk();
> +}
> +

OK.

> diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
> index 1c0e8659d9..3be01316e8 100755
> --- a/t/t6120-describe.sh
> +++ b/t/t6120-describe.sh
> @@ -310,6 +310,21 @@ test_expect_success 'describe ignoring a borken 
> submodule' '
>   grep broken out
>  '
>  
> +test_expect_success 'describe a blob at a tag' '
> + echo "make it a unique blob" >file &&
> + git add file && git commit -m "content in file" &&
> + git tag -a -m "latest annotated tag" unique-file &&
> + git describe HEAD:file >actual &&
> + echo "unique-file:file" >expect &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'describe a surviving blob' '
> + git commit --allow-empty -m "empty commit" &&
> + git describe HEAD:file >actual &&
> + grep unique-file-1-g actual
> +'
> +

I am not sure what "surviving" means in this context.  The word
hinted that the test would be finding a blob that only appears in a
commit that only appears as a reflog entry, but that wasn't the
case, which was a bit disappointing.

Re: [PATCHv2 2/7] revision.h: introduce blob/tree walking in order of the commits

[Junio C Hamano]

Stefan Beller  writes:

> diff --git a/t/t6100-rev-list-in-order.sh b/t/t6100-rev-list-in-order.sh
> new file mode 100755
> index 00..651666979b
> --- /dev/null
> +++ b/t/t6100-rev-list-in-order.sh
> @@ -0,0 +1,46 @@
> +#!/bin/sh
> +
> +test_description='miscellaneous rev-list tests'
> +
> +. ./test-lib.sh
> +
> +
> +test_expect_success 'setup' '
> + for x in one two three four
> + do
> + echo $x >$x &&
> + git add $x &&
> + git commit -m "add file $x"
> + done &&
> + for x in four three
> + do
> + git rm $x &&
> + git commit -m "remove $x"
> + done &&

When "git commit -m 'remove four'" fails, this loop would still
continue, so the &&-chain in "done &&" would still be rendered
ineffetive.

> + git rev-list --in-commit-order --objects HEAD >actual.raw &&
> + cut -c 1-40 >actual  +
> + git cat-file --batch-check="%(objectname)" >expect.raw <<-\EOF &&
> + HEAD^{commit}
> + HEAD^{tree}
> + HEAD^{tree}:one
> + HEAD^{tree}:two
> + HEAD~1^{commit}
> + HEAD~1^{tree}
> + HEAD~1^{tree}:three
> + HEAD~2^{commit}
> + HEAD~2^{tree}
> + HEAD~2^{tree}:four
> + HEAD~3^{commit}
> + # HEAD~3^{tree} skipped
> + HEAD~4^{commit}
> + # HEAD~4^{tree} skipped
> + HEAD~5^{commit}
> + HEAD~5^{tree}
> + EOF
> + grep -v "#" >expect  +
> + test_cmp expect actual
> +'
> +
> +test_done

Re: [PATCHv2 1/7] list-objects.c: factor out traverse_trees_and_blobs

[Junio C Hamano]

Stefan Beller  writes:

> With traverse_trees_and_blobs factored out of the main traverse function,
> the next patch can introduce an in-order revision walking with ease.
>
> In the next patch we'll call `traverse_trees_and_blobs` from within the
> loop walking the commits, such that we'll have one invocation of that
> function per commit.  That is why we do not want to have memory allocations
> in that function, such as we'd have if we were to use a strbuf locally.
> Pass a strbuf from traverse_commit_list into the blob and tree traversing
> function as a scratch pad that only needs to be allocated once.

Makes sense.

I still don't think base_path is any clearer than base that was used
in the original, though.

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

[Junio C Hamano]

Stefan Beller  writes:

> Maybe
>
>   "git-describe - Describe a blob or commit using graph relations"
>
> though that sounds too generic, but it is accurate as all we do is
> a heuristic for graph walk ways.

We used to describe commit using commit ancestry (i.e. finding the
place where a "wanted" commit sits in the commit ancestry graph).
Now we are extending it to describe an object by finding the place
where it sits in the graph that shows object reachability relation,
but saying "graph" without saying "graph over what" is probably not
descriptive enough for most readers.

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

[Junio C Hamano]

Stefan Beller  writes:

>>  Given the difficulty in
>> coming up with the single-liner description of what it does we saw
>> above, I suspect that splitting SYNOPSIS out into two very distinct
>> operating mode might make it easier to read.
>>
>> SYNOPSIS
>> 
>> [verse]
>> 'git describe' [--all] [--tags] [--contains] [--abbrev=] 
>> [...]
>>+'git describe' [...] ...
>>
>> Then this additional paragraph can say "When describin a ",
>> without using a (technically nonsense) phrase "if 
>> refers to a blob", which is never true.
>
> ok, do we know about 'blob-ish' as a term?

No, and I do not think there is any need to say -ish at all for this
use case.

After all, when we accept a  when a  is called
for, that is only because there is only one way to use the commit in
place of the wanted ; we take the top-level tree contained in
it.  You cannot say you take  and take a , as it is
unclear which entry in the  can act as the substitute for the
wanted .

You accept blob object name in this mode, so just saying  is
sufficient.

Re: [PATCH v3 7/8] diff: remove DIFF_OPT_CLR macro

[Junio C Hamano]

Stefan Beller  writes:

> On Tue, Oct 31, 2017 at 11:19 AM, Brandon Williams  wrote:
>> Remove the `DIFF_OPT_SET` macro and instead set the flags directly.
>
>  _CLR here

Will squash in. Thanks.

Re: Meaning of two commit-ish hash in git diff

[Junio C Hamano]

Yubin Ruan  writes:

>>> IOW, if you have the contents of the blob whose object name is
>>> f8886b4, by applying this patch, you will get a blob whose object
>>> name is a1c96df.
>>>
>>> The information is used by "git am -3" when the patch does not apply
>>> cleanly to fall back to the 3-way merge.
>> 
>> The ability to 'git describe` those blob object IDs is currently the subject 
>> of a patch series
>> https://public-inbox.org/git/20171031211852.13001-1-sbel...@google.com/
>> 
>> Maybe see if would have helped ;-)
>
> What would be other use case of the blob object / blob name ?

Perhaps start from "git cat-file blob $name".  And follow the path
your imagination leads you to ;-)

Re: [PATCH 1/2] quote-email populates the fields

[Junio C Hamano]

Payre Nathan  writes:

> From: Tom Russello 
>
> ---

Missing something here???

>  Documentation/git-send-email.txt |   3 +
>  git-send-email.perl  |  70 ++-
>  t/t9001-send-email.sh| 117 
> +--
>  3 files changed, 147 insertions(+), 43 deletions(-)
>
> diff --git a/Documentation/git-send-email.txt 
> b/Documentation/git-send-email.txt
> index bac9014ac..710b5ff32 100644
> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -106,6 +106,9 @@ illustration below where `[PATCH v2 0/3]` is in reply to 
> `[PATCH 0/2]`:
>  Only necessary if --compose is also set.  If --compose
>  is not set, this will be prompted for.
>  
> +--quote-email=::
> + Fill appropriately header fields for the reply to the given email.
> +

The cover letter said:

This patch allows send-email to do most of the job for the user, who can
now save the email to a file and use:

  git send-email --quote-email=

"To" and "Cc" will be added automaticaly and the email quoted.
It's possible to edit the email before sending with --compose.

and I somehow expected to see the body of the e-mail this option is
"quoting" to be also inserted in the text.  After all, that is what
"quote" means.

But the description above (and the code below, judging from the way
the reading from $fh that was opened form $quote_email stops at the
first blank line, aka end of header) says what is happening is quite
different.  The contents of the file is used to extract what the
user would have given to --cc/--to/--in-reply-to from the command
line by looking at it, if this option were not available.

I personally prefer the "pick up the header information so that the
user do not have to formulate the command line options" behaviour
that does *NOT* quote the body of the message into the outgoing
message.  So:

 * Do not call this option "quote" anything; you are not quoting,
   just using some info from the given file.  

   I wonder if we can simply reuse "--in-reply-to" option for this
   purpose.  If it is a message id and not a file on the filesystem,
   we behave just as before.  Otherwise we try to open it as a file
   and grab the "Message-ID:" header from it and use it.

 * The description "Fill *appropriately* header fileds" is useless,
   as what looks "appropriate" to you is not clear/known to
   readers.  Instead, say what header is filled with what
   information (e.g. "find Message-Id: and place its value on
   In-Reply-To: header").

   For that matter, "To and CC will be added automatically" in the
   coer letter is still vague; are you reading To/CC in the given
   file and placing their values on some (unnamed) header of the
   outgoing message?  Or are you reading some (unnamed) header in
   the given file and placing their values on To/CC header of the
   outging message?

> diff --git a/git-send-email.perl b/git-send-email.perl
> index 2208dcc21..665c47d15 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -57,6 +57,7 @@ git send-email --dump-aliases
>  --[no-]bcc* Email Bcc:
>  --subject * Email "Subject:"
>  --in-reply-to * Email "In-Reply-To:"
> +--quote-email* Populate header fields appropriately.

Likewise.  If what's "appropriate" is clear to the readers, the word
in this description adds no value because everybody would know how
fields are populated.  Otherwise, it does not add any value because
everybody would have no clue how fields are populated.

> @@ -652,6 +654,70 @@ if (@files) {
>   usage();
>  }
>  
> +if ($quote_email) {
> + my $error = validate_patch($quote_email);
> + die "fatal: $quote_email: $error\nwarning: no patches were sent\n"
> + if $error;

validate_patch() calls sendemail-validate hook that is expecting to
be fed a patch email you are going to send out that might have
errors so that it can catch it and save you from embarrassment.  The
file you are feeding it is *NOT* what you are going to send out, but
is what you are responding to with your patch.  Even if it had an
embarassing error as a patch, that is not something you care about
(and it is something you received, so catching this late won't save
the sender from embarrassment anyway).

> +
> + my @header = ();
> +
> + open my $fh, "<", $quote_email or die "can't open file $quote_email";
> +
> + # Get the email header
> + while (<$fh>) {
> + # Turn crlf line endings into lf-only
> + s/\r//g;
> + last if /^\s*$/;
> + if (/^\s+\S/ and @header) {

I wonder how significant this requirement to have at least one "\S"
on the line is.  I know you copied this from the main sending
loop, so this is not a new issue and not something we may want to
fix in this patch.

> + chomp($header[$#header]);

Re: [PATCH] t5580: add Cygwin support

[Junio C Hamano]

Adam Dinwoodie  writes:

> t5580 tests that specifying Windows UNC paths works with Git.  Cygwin
> supports UNC paths, albeit only using forward slashes, not backslashes,
> so run the compatible tests on Cygwin as well as MinGW.
>
> The only complication is Cygwin's `pwd`, which returns a *nix-style
> path, and that's not suitable for calculating the UNC path to the
> current directory.  Instead use Cygwin's `cygpath` utility to get the
> Windows-style path.
>
> Signed-off-by: Adam Dinwoodie 
> ---
>  t/t5580-clone-push-unc.sh | 12 
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
> index b322c2f72..47a9a7cda 100755
> --- a/t/t5580-clone-push-unc.sh
> +++ b/t/t5580-clone-push-unc.sh
> @@ -3,12 +3,16 @@
>  test_description='various Windows-only path tests'
>  . ./test-lib.sh
>  
> -if ! test_have_prereq MINGW; then
> +if test_have_prereq CYGWIN; then
> + alias winpwd='cygpath -aw .'
> +elif test_have_prereq MINGW; then
> + alias winpwd=pwd
> +else
>   skip_all='skipping Windows-only path tests'
>   test_done
>  fi

The fact that UNCPATH matters only on windows-related platforms
justifies the name used for this alias, I guess ;-)

>  
> -UNCPATH="$(pwd)"
> +UNCPATH="$(winpwd)"
>  case "$UNCPATH" in
>  [A-Z]:*)
>   # Use administrative share e.g. \\localhost\C$\git-sdk-64\usr\src\git
> @@ -45,8 +49,8 @@ test_expect_success push '
>   test "$rev" = "$(git rev-parse --verify refs/heads/to-push)"
>  '
>  
> -test_expect_success 'remote nick cannot contain backslashes' '
> - BACKSLASHED="$(pwd | tr / )" &&
> +test_expect_success MINGW 'remote nick cannot contain backslashes' '
> + BACKSLASHED="$(winpwd | tr / )" &&
>   git ls-remote "$BACKSLASHED" >out 2>err &&
>   test_i18ngrep ! "unable to access" err
>  '

Re: [PATCH v2 2/4] Add structure representing hash algorithm

[brian m. carlson]

On Mon, Oct 30, 2017 at 04:36:15PM -0700, Brandon Williams wrote:
> On 10/28, brian m. carlson wrote:
> > 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.
> > 
> > Signed-off-by: brian m. carlson 
> > ---
> > I believe I did get the format_id constant for SHA-1 right, but
> > sanity-checking would be appreciated.  We don't want another Referer
> > mess.
> > 
> >  cache.h | 55 +++
> >  sha1_file.c | 43 +++
> >  2 files changed, 98 insertions(+)
> > 
> > diff --git a/cache.h b/cache.h
> > index 6440e2bf21..9e9eb08f05 100644
> > --- a/cache.h
> > +++ b/cache.h
> 
> Maybe it would be good to place this interface in its own header file
> that way we can avoid cluttering cache.h with more stuff?

Sure, if you like.  It will end up needing to be included in cache.h
because of the blob and tree code, but I'm happy to split it out.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH v3 2/8] diff: convert flags to be stored in bitfields

[Junio C Hamano]

Stefan Beller  writes:

> After some quick research our coding style on bit fields is twofold:
> Most older code is this way and more recent code seems to prefer
>
> unsigned  SP : SP ;

Yes, we are very inconsistent.  What does the clang format rules
Brandon came up with have to say on this?  FWIW, checkpatch.pl is
unhappy without spaces on both side.

>> +static inline void diff_flags_or(struct diff_flags *a,
>> +const struct diff_flags *b)
>> +{
>> +   char *tmp_a = (char *)a;
>> +   const char *tmp_b = (const char *)b;
>> +   int i;
>> +
>> +   for (i = 0; i < sizeof(struct diff_flags); i++)
>
> I think most of the code prefers to put the variable into the sizeof
> argument i.e. 'sizeof(*a)', as that is presumably more maintainable.
> (If the type of 'a' changes, then we don't forget to adapt this place,
> but the compiler can take care of it.

Yup, but in this case we won't change the type, no?

Re: What's cooking in git.git (Oct 2017, #07; Mon, 30)

[Junio C Hamano]

Jeff Hostetler  writes:

> From: Junio C Hamano [mailto:gits...@pobox.com] 
> Subject: Re: What's cooking in git.git (Oct 2017, #07; Mon, 30)
>
>> Jeff Hostetler  writes:
>> 
>>> I've been assuming that the jt/partial-clone-lazy-fetch is a 
>>> placeholder for our next combined patch series.
>>
>> Yes, that, together with the expectation that I will hear from both you and 
>> JTan 
>> once the result of combined effort becomes ready to replace this 
>> placeholder, 
>> matches my assumption.
>> 
>> Is that happening now?
>
> Yes, I'm merging our them now and hope to have a version to
> send to Jonathan and/or the list sometime this week.

Thanks.

Re: [PATCH 7/7] t6120: fix typo in test name

[Junio C Hamano]

Stefan Beller  writes:

> Signed-off-by: Stefan Beller 
> ---
>  t/t6120-describe.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Good.  I am guessing that you are sending this as the last/optional
one because this was found _after_ you worked on other parts of the
series, but I think it is easier to reason about if this were marked
as a preliminary clean-up and moved to the front of the series.

Thanks.

>
> diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
> index 3be01316e8..fd329f173a 100755
> --- a/t/t6120-describe.sh
> +++ b/t/t6120-describe.sh
> @@ -304,7 +304,7 @@ test_expect_success 'describe chokes on severely broken 
> submodules' '
>   mv .git/modules/sub1/ .git/modules/sub_moved &&
>   test_must_fail git describe --dirty
>  '
> -test_expect_success 'describe ignoring a borken submodule' '
> +test_expect_success 'describe ignoring a broken submodule' '
>   git describe --broken >out &&
>   test_when_finished "mv .git/modules/sub_moved .git/modules/sub1" &&
>   grep broken out

Re: Meaning of two commit-ish hash in git diff

[Yubin Ruan]

Thanks Philip,

On 11/01/2017 08:27 AM, Philip Oakley wrote:
> Yubin ,
> 
> From: "Junio C Hamano" 
>> Yubin Ruan  writes:
>>
>>> diff --git a/path/somefile b/path/somefile
>>> index f8886b4..a1c96df 100644
>>> --- a/path/somefile
>>> +++ b/path/somefile
>>> 
>>>
>>> This is output by a `git diff` between two adjacent commits but they are
>>> not any commit hash. I grep through the whole $(git log) but still cannot
>>> find those hash.
>>
>> The f8886b4 you see on the left is the name of the blob object on
>> the left hand side of the comparison that produced this output;
>> similarly a1c96df is the name of the blob object on the right hand
>> side of the comparison.
>>
>> IOW, if you have the contents of the blob whose object name is
>> f8886b4, by applying this patch, you will get a blob whose object
>> name is a1c96df.
>>
>> The information is used by "git am -3" when the patch does not apply
>> cleanly to fall back to the 3-way merge.
> 
> The ability to 'git describe` those blob object IDs is currently the subject 
> of a patch series
> https://public-inbox.org/git/20171031211852.13001-1-sbel...@google.com/
> 
> Maybe see if would have helped ;-)

What would be other use case of the blob object / blob name ?

Yubin

Re: Meaning of two commit-ish hash in git diff

[Philip Oakley]

Yubin ,

From: "Junio C Hamano" 

Yubin Ruan  writes:


diff --git a/path/somefile b/path/somefile
index f8886b4..a1c96df 100644
--- a/path/somefile
+++ b/path/somefile


This is output by a `git diff` between two adjacent commits but they are
not any commit hash. I grep through the whole $(git log) but still cannot
find those hash.


The f8886b4 you see on the left is the name of the blob object on
the left hand side of the comparison that produced this output;
similarly a1c96df is the name of the blob object on the right hand
side of the comparison.

IOW, if you have the contents of the blob whose object name is
f8886b4, by applying this patch, you will get a blob whose object
name is a1c96df.

The information is used by "git am -3" when the patch does not apply
cleanly to fall back to the 3-way merge.


The ability to 'git describe` those blob object IDs is currently the subject 
of a patch series

https://public-inbox.org/git/20171031211852.13001-1-sbel...@google.com/

Maybe see if would have helped ;-)

Philip 

[PATCH] sequencer: pass absolute GIT_DIR to exec commands

[Jacob Keller]

From: Jacob Keller 

When we replaced the old shell script based interactive rebase in
commmit 18633e1a22a6 ("rebase -i: use the rebase--helper builtin",
2017-02-09) we introduced a regression of functionality in that the
GIT_DIR would be sent to the environment of the exec command as-is.

This generally meant that it would be passed as "GIT_DIR=.git", which
causes problems for any exec command that wants to run git commands in
a subdirectory.

This isn't a very large regression, since it is not that likely that the
exec command will run a git command, and even less likely that it will
need to do so in a subdir. This regression was discovered by a build
system which uses git-describe to find the current version of the build
system, and happened to do so from the src/ sub directory of the
project.

Fix this by passing in the absolute path of the git directory into the
child environment.

Signed-off-by: Jacob Keller 
---
 sequencer.c   |  7 ++-
 t/t3404-rebase-interactive.sh | 11 +++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index f2a10cc4f24c..332a383b037d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1862,12 +1862,15 @@ static int error_failed_squash(struct commit *commit,
 
 static int do_exec(const char *command_line)
 {
+   struct argv_array child_env = ARGV_ARRAY_INIT;
const char *child_argv[] = { NULL, NULL };
int dirty, status;
 
fprintf(stderr, "Executing: %s\n", command_line);
child_argv[0] = command_line;
-   status = run_command_v_opt(child_argv, RUN_USING_SHELL);
+   argv_array_pushf(_env, "GIT_DIR=%s", 
absolute_path(get_git_dir()));
+   status = run_command_v_opt_cd_env(child_argv, RUN_USING_SHELL, NULL,
+ child_env.argv);
 
/* force re-reading of the cache */
if (discard_cache() < 0 || read_cache() < 0)
@@ -1897,6 +1900,8 @@ static int do_exec(const char *command_line)
status = 1;
}
 
+   argv_array_clear(_env);
+
return status;
 }
 
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 3704dbb2ecf6..6a82d1ed876d 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -108,6 +108,17 @@ test_expect_success 'rebase -i with the exec command runs 
from tree root' '
rm -fr subdir
 '
 
+test_expect_success 'rebase -i with exec allows git commands in subdirs' '
+   test_when_finished "rm -rf subdir" &&
+   test_when_finished "git rebase --abort ||:" &&
+   git checkout master &&
+   mkdir subdir && (cd subdir &&
+   set_fake_editor &&
+   FAKE_LINES="1 exec_cd_subdir_&&_git_rev-parse_--is-inside-work-tree" \
+   git rebase -i HEAD^
+   )
+'
+
 test_expect_success 'rebase -i with the exec command checks tree cleanness' '
git checkout master &&
set_fake_editor &&
-- 
2.11.1.4.gad8c7cd

RE: Is it possible to convert a Json file to xml file with Git

[Randall S. Becker]

> On October 31, 2017 5:23 PM, Kevin Daudt wrote:
> > On Tue, Oct 31, 2017 at 05:28:40PM +, Eyjolfur Eyjolfsson wrote:
> > I have a question.
> > Is it possible to convert a Json file to XML with Git
> 
> git is a version control system, which is mostly content agnostic. It
knows
> nothing about json or xml, let alone how to convert them.
> 
> You might want to use some kind of programming language to do the
> conversion.

Speculating... one possible reason to do this is during a protocol
conversion effort, where definitions are moving from XML to JSON form. In
legacy VCS systems, keeping interface definitions in one file and converting
the content may be important. However, in git, with its concept of atomicity
(multiple files are committed in a single version across the whole
repository), dropping one file (e.g., XML) and adding another (e.g., JSON),
can be done in one commit, and never lost or confused as to what is
intended. This makes git ideal for modernization and evolutionary projects.

If, however, there is an application or systemic requirement to change the
content of a file from XML to JSON without changing the name - I've seen it
happen - you may want to consider building a smudge filter that understands
the difference and maps between the two, to allow git diff operations
between old and new formats. I would not recommend using this approach
except as a last possible resort. Make a new file as Kevin intimated.

Just Musing on the Topic,
Randall

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

Re: [PATCHv2 6/7] builtin/describe.c: describe a blob

[Eric Sunshine]

On Tue, Oct 31, 2017 at 5:18 PM, Stefan Beller  wrote:
> When describing commits, we try to anchor them to tags or refs, as these
> are conceptually on a higher level than the commit. And if there is no ref
> or tag that matches exactly, we're out of luck.  So we employ a heuristic
> to make up a name for the commit. These names are ambivalent, there might

I guess you meant s/ambivalent/ambiguous/ ?

> be different tags or refs to anchor to, and there might be different
> path in the DAG to travel to arrive at the commit precisely.
>
> [1] https://stackoverflow.com/questions/223678/which-commit-has-this-blob
> Signed-off-by: Stefan Beller 

Blank line before sign-off.

> ---
> diff --git a/builtin/describe.c b/builtin/describe.c
> @@ -445,11 +495,16 @@ static void describe(const char *arg, int last_one)
>
> if (get_oid(arg, ))
> die(_("Not a valid object name %s"), arg);
> -   cmit = lookup_commit_reference();
> -   if (!cmit)
> -   die(_("%s is not a valid '%s' object"), arg, commit_type);
> +   cmit = lookup_commit_reference_gently(, 1);
>
> -   describe_commit(, );
> +   if (cmit) {
> +   describe_commit(, );
> +   } else {
> +   if (lookup_blob())
> +   describe_blob(oid, );
> +   else
> +   die(_("%s is neither a commit nor blob"), arg);
> +   }

Not at all worth a re-roll, but less nesting and a bit less noisy:

if (cmt)
describe_commit(...);
else if (lookup_blob(...))
describe_blob(...);
else
die(...);

Re: [PATCH v3 0/8] convert diff flags to be stored in a struct

[Stefan Beller]

On Tue, Oct 31, 2017 at 11:19 AM, Brandon Williams  wrote:
> Changes in v3:
>  * Now always pass struct diff_flags by reference and don't return the struct
>but rather modify the passed in struct.
>  * Don't clear TEXTCONV_SET_VIA_CMDLINE when --no-textconv is passed
>  * added additional patches (set out separately before) to remove the macros
>and change the struct members to lowercase

I have reviewed this version and the functionality introduced
looks good to me. My only nits were regarding style and typos.

Thanks,
Stefan

Re: [PATCH v3 7/8] diff: remove DIFF_OPT_CLR macro

[Stefan Beller]

On Tue, Oct 31, 2017 at 11:19 AM, Brandon Williams  wrote:
> Remove the `DIFF_OPT_SET` macro and instead set the flags directly.

 _CLR here

Re: [PATCH v3 2/8] diff: convert flags to be stored in bitfields

[Stefan Beller]

On Tue, Oct 31, 2017 at 11:19 AM, Brandon Williams  wrote:
> We cannot add many more flags to the diff machinery due to the
> limitations of the number of flags that can be stored in a single
> unsigned int.  In order to allow for more flags to be added to the diff
> machinery in the future this patch converts the flags to be stored in
> bitfields in 'struct diff_flags'.
>
> Signed-off-by: Brandon Williams 

Thanks for this cleanup series!
Stefan

> +struct diff_flags {
> +   unsigned RECURSIVE:1;

After some quick research our coding style on bit fields is twofold:
Most older code is this way and more recent code seems to prefer

unsigned  SP : SP ;

If this turns out to be the only nit, I would ignore it for the sake of
faster settlement of the series.


> +static inline void diff_flags_or(struct diff_flags *a,
> +const struct diff_flags *b)
> +{
> +   char *tmp_a = (char *)a;
> +   const char *tmp_b = (const char *)b;
> +   int i;
> +
> +   for (i = 0; i < sizeof(struct diff_flags); i++)

I think most of the code prefers to put the variable into the sizeof
argument i.e. 'sizeof(*a)', as that is presumably more maintainable.
(If the type of 'a' changes, then we don't forget to adapt this place,
but the compiler can take care of it.

Re: [PATCHv2 4/7] builtin/describe.c: print debug statements earlier

[Eric Sunshine]

On Tue, Oct 31, 2017 at 5:18 PM, Stefan Beller  wrote:
> For debuggers aid we'd want to print debug statements early, so
> introduce a new line in the debug output that describes the whole
> function, and the change the next debug output to describe why we need

s/and the/and/
...or...
s/and the/and then/

> to search. Conveniently drop the arg from the second line; which will
> be useful in a follow up commit, that refactors the describe function.
>
> Signed-off-by: Stefan Beller 

Re: Is it possible to convert a Json file to xml file with Git

[Kevin Daudt]

On Tue, Oct 31, 2017 at 05:28:40PM +, Eyjolfur Eyjolfsson wrote:
> Hi
> 
> I have a question.
> Is it possible to convert a Json file to XML with Git
> 
> Best regards
> 
> Eyjolfur Eyjolfsson
> 
> (e) eyjolfureyjolfs...@tprg.com
> (w) tpretailgroup.com
> 

Hello Eyjolfur,

git is a version control system, which is mostly content agnostic. It
knows nothing about json or xml, let alone how to convert them.

You might want to use some kind of programming language to do the
conversion.

Could you perhaps explain more why you are asking this question?

Kevin.

[PATCHv2 7/7] t6120: fix typo in test name

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 t/t6120-describe.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 3be01316e8..fd329f173a 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -304,7 +304,7 @@ test_expect_success 'describe chokes on severely broken 
submodules' '
mv .git/modules/sub1/ .git/modules/sub_moved &&
test_must_fail git describe --dirty
 '
-test_expect_success 'describe ignoring a borken submodule' '
+test_expect_success 'describe ignoring a broken submodule' '
git describe --broken >out &&
test_when_finished "mv .git/modules/sub_moved .git/modules/sub1" &&
grep broken out
-- 
2.15.0.rc2.443.gfcc3b81c0a

[PATCHv2 4/7] builtin/describe.c: print debug statements earlier

[Stefan Beller]

For debuggers aid we'd want to print debug statements early, so
introduce a new line in the debug output that describes the whole
function, and the change the next debug output to describe why we need
to search. Conveniently drop the arg from the second line; which will
be useful in a follow up commit, that refactors the describe function.

Signed-off-by: Stefan Beller 
---
 builtin/describe.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index fd61f463cf..3136efde31 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -293,6 +293,9 @@ static void describe(const char *arg, int last_one)
unsigned long seen_commits = 0;
unsigned int unannotated_cnt = 0;
 
+   if (debug)
+   fprintf(stderr, _("describe %s\n"), arg);
+
if (get_oid(arg, ))
die(_("Not a valid object name %s"), arg);
cmit = lookup_commit_reference();
@@ -316,7 +319,7 @@ static void describe(const char *arg, int last_one)
if (!max_candidates)
die(_("no tag exactly matches '%s'"), 
oid_to_hex(>object.oid));
if (debug)
-   fprintf(stderr, _("searching to describe %s\n"), arg);
+   fprintf(stderr, _("No exact match on refs or tags, searching to 
describe\n"));
 
if (!have_util) {
struct hashmap_iter iter;
-- 
2.15.0.rc2.443.gfcc3b81c0a

[PATCHv2 6/7] builtin/describe.c: describe a blob

[Stefan Beller]

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

"This is an interesting endeavor, because describing things is hard."
  -- me, upon writing this patch.

When describing commits, we try to anchor them to tags or refs, as these
are conceptually on a higher level than the commit. And if there is no ref
or tag that matches exactly, we're out of luck.  So we employ a heuristic
to make up a name for the commit. These names are ambivalent, there might
be different tags or refs to anchor to, and there might be different
path in the DAG to travel to arrive at the commit precisely.

When describing a blob, we want to describe the blob from a higher layer
as well, which is a tuple of (commit, deep/path) as the tree objects
involved are rather uninteresting.  The same blob can be referenced by
multiple commits, so how we decide which commit to use?  This patch
implements a rather naive approach on this: As there are no back pointers
from blobs to commits in which the blob occurs, we'll start walking from
any tips available, listing the blobs in-order of the commit and once we
found the blob, we'll take the first commit that listed the blob.  For
source code this is likely not the first commit that introduced the blob,
but rather the latest commit that contained the blob.  For example:

  git describe v0.99:Makefile
  v0.99-5-gab6625e06a:Makefile

tells us the latest commit that contained the Makefile as it was in tag
v0.99 is commit v0.99-5-gab6625e06a (and at the same path), as the next
commit on top v0.99-6-gb1de9de2b9 ([PATCH] Bootstrap "make dist",
2005-07-11) touches the Makefile.

Let's see how this description turns out, if it is useful in day-to-day
use as I have the intuition that we'd rather want to see the *first*
commit that this blob was introduced to the repository (which can be
achieved easily by giving the `--reverse` flag in the describe_blob rev
walk).

[1] https://stackoverflow.com/questions/223678/which-commit-has-this-blob
Signed-off-by: Stefan Beller 
---
 Documentation/git-describe.txt | 13 -
 builtin/describe.c | 65 ++
 t/t6120-describe.sh| 15 ++
 3 files changed, 87 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index c924c945ba..077c3c2193 100644
--- a/Documentation/git-describe.txt
+++ b/Documentation/git-describe.txt
@@ -3,7 +3,7 @@ git-describe(1)
 
 NAME
 
-git-describe - Describe a commit using the most recent tag reachable from it
+git-describe - Describe a commit or blob using the graph relations
 
 
 SYNOPSIS
@@ -11,6 +11,7 @@ SYNOPSIS
 [verse]
 'git describe' [--all] [--tags] [--contains] [--abbrev=] [...]
 'git describe' [--all] [--tags] [--contains] [--abbrev=] --dirty[=]
+'git describe' [] 
 
 DESCRIPTION
 ---
@@ -24,6 +25,16 @@ By default (without --all or --tags) `git describe` only 
shows
 annotated tags.  For more information about creating annotated tags
 see the -a and -s options to linkgit:git-tag[1].
 
+If the given object refers to a blob, it will be described
+as `:`, such that the blob can be found
+at `` in the ``. Note, that the commit is likely
+not the commit that introduced the blob, but the one that was found
+first; to find the commit that introduced the blob, you need to find
+the commit that last touched the path, e.g.
+`git log  -- `.
+As blobs do not point at the commits they are contained in,
+describing blobs is slow as we have to walk the whole graph.
+
 OPTIONS
 ---
 ...::
diff --git a/builtin/describe.c b/builtin/describe.c
index 9e9a5ed5d4..382cbae908 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -3,6 +3,7 @@
 #include "lockfile.h"
 #include "commit.h"
 #include "tag.h"
+#include "blob.h"
 #include "refs.h"
 #include "builtin.h"
 #include "exec_cmd.h"
@@ -11,8 +12,9 @@
 #include "hashmap.h"
 #include "argv-array.h"
 #include "run-command.h"
+#include "revision.h"
+#include "list-objects.h"
 
-#define SEEN   (1u << 0)
 #define MAX_TAGS   (FLAG_BITS - 1)
 
 static const char * const describe_usage[] = {
@@ -434,6 +436,54 @@ static void describe_commit(struct object_id *oid, struct 
strbuf *dst)
strbuf_addstr(dst, suffix);
 }
 
+struct process_commit_data {
+   struct object_id current_commit;
+   struct object_id looking_for;
+   struct strbuf *dst;
+};
+
+static void process_commit(struct commit *commit, void *data)
+{
+   struct process_commit_data *pcd = data;
+   pcd->current_commit = commit->object.oid;
+}
+
+static void process_object(struct object *obj, const char *path, void *data)
+{
+   struct process_commit_data *pcd = data;
+
+   if (!oidcmp(>looking_for, >oid) && !pcd->dst->len) {
+   reset_revision_walk();
+   describe_commit(>current_commit, pcd->dst);
+  

[PATCHv2 3/7] builtin/describe.c: rename `oid` to avoid variable shadowing

[Stefan Beller]

The function `describe` has already a variable named `oid` declared at
the beginning of the function for an object id.  Do not shadow that
variable with a pointer to an object id.

Signed-off-by: Stefan Beller 
---
 builtin/describe.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index 29075dbd0f..fd61f463cf 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -381,9 +381,9 @@ static void describe(const char *arg, int last_one)
}
 
if (!match_cnt) {
-   struct object_id *oid = >object.oid;
+   struct object_id *cmit_oid = >object.oid;
if (always) {
-   printf("%s", find_unique_abbrev(oid->hash, abbrev));
+   printf("%s", find_unique_abbrev(cmit_oid->hash, 
abbrev));
if (suffix)
printf("%s", suffix);
printf("\n");
@@ -392,11 +392,11 @@ static void describe(const char *arg, int last_one)
if (unannotated_cnt)
die(_("No annotated tags can describe '%s'.\n"
"However, there were unannotated tags: try 
--tags."),
-   oid_to_hex(oid));
+   oid_to_hex(cmit_oid));
else
die(_("No tags can describe '%s'.\n"
"Try --always, or create some tags."),
-   oid_to_hex(oid));
+   oid_to_hex(cmit_oid));
}
 
QSORT(all_matches, match_cnt, compare_pt);
-- 
2.15.0.rc2.443.gfcc3b81c0a

[PATCHv2 5/7] builtin/describe.c: factor out describe_commit

[Stefan Beller]

In the next patch we'll learn how to describe more than just commits,
so factor out describing commits into its own function.  That will make
the next patches easy as we still need to describe a commit as part of
describing blobs.

While factoring out the functionality to describe_commit, make sure
that any output to stdout is put into a strbuf, which we can print
afterwards, using puts which also adds the line ending.

Signed-off-by: Stefan Beller 
---
 builtin/describe.c | 63 --
 1 file changed, 37 insertions(+), 26 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index 3136efde31..9e9a5ed5d4 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -256,7 +256,7 @@ static unsigned long finish_depth_computation(
return seen_commits;
 }
 
-static void display_name(struct commit_name *n)
+static void append_name(struct commit_name *n, struct strbuf *dst)
 {
if (n->prio == 2 && !n->tag) {
n->tag = lookup_tag(>oid);
@@ -272,19 +272,18 @@ static void display_name(struct commit_name *n)
}
 
if (n->tag)
-   printf("%s", n->tag->tag);
+   strbuf_addstr(dst, n->tag->tag);
else
-   printf("%s", n->path);
+   strbuf_addstr(dst, n->path);
 }
 
-static void show_suffix(int depth, const struct object_id *oid)
+static void append_suffix(int depth, const struct object_id *oid, struct 
strbuf *dst)
 {
-   printf("-%d-g%s", depth, find_unique_abbrev(oid->hash, abbrev));
+   strbuf_addf(dst, "-%d-g%s", depth, find_unique_abbrev(oid->hash, 
abbrev));
 }
 
-static void describe(const char *arg, int last_one)
+static void describe_commit(struct object_id *oid, struct strbuf *dst)
 {
-   struct object_id oid;
struct commit *cmit, *gave_up_on = NULL;
struct commit_list *list;
struct commit_name *n;
@@ -293,26 +292,18 @@ static void describe(const char *arg, int last_one)
unsigned long seen_commits = 0;
unsigned int unannotated_cnt = 0;
 
-   if (debug)
-   fprintf(stderr, _("describe %s\n"), arg);
-
-   if (get_oid(arg, ))
-   die(_("Not a valid object name %s"), arg);
-   cmit = lookup_commit_reference();
-   if (!cmit)
-   die(_("%s is not a valid '%s' object"), arg, commit_type);
+   cmit = lookup_commit_reference(oid);
 
n = find_commit_name(>object.oid);
if (n && (tags || all || n->prio == 2)) {
/*
 * Exact match to an existing ref.
 */
-   display_name(n);
+   append_name(n, dst);
if (longformat)
-   show_suffix(0, n->tag ? >tag->tagged->oid : );
+   append_suffix(0, n->tag ? >tag->tagged->oid : oid, 
dst);
if (suffix)
-   printf("%s", suffix);
-   printf("\n");
+   strbuf_addstr(dst, suffix);
return;
}
 
@@ -386,10 +377,9 @@ static void describe(const char *arg, int last_one)
if (!match_cnt) {
struct object_id *cmit_oid = >object.oid;
if (always) {
-   printf("%s", find_unique_abbrev(cmit_oid->hash, 
abbrev));
+   strbuf_addstr(dst, find_unique_abbrev(cmit_oid->hash, 
abbrev));
if (suffix)
-   printf("%s", suffix);
-   printf("\n");
+   strbuf_addstr(dst, suffix);
return;
}
if (unannotated_cnt)
@@ -437,15 +427,36 @@ static void describe(const char *arg, int last_one)
}
}
 
-   display_name(all_matches[0].name);
+   append_name(all_matches[0].name, dst);
if (abbrev)
-   show_suffix(all_matches[0].depth, >object.oid);
+   append_suffix(all_matches[0].depth, >object.oid, dst);
if (suffix)
-   printf("%s", suffix);
-   printf("\n");
+   strbuf_addstr(dst, suffix);
+}
+
+static void describe(const char *arg, int last_one)
+{
+   struct object_id oid;
+   struct commit *cmit;
+   struct strbuf sb = STRBUF_INIT;
+
+   if (debug)
+   fprintf(stderr, _("describe %s\n"), arg);
+
+   if (get_oid(arg, ))
+   die(_("Not a valid object name %s"), arg);
+   cmit = lookup_commit_reference();
+   if (!cmit)
+   die(_("%s is not a valid '%s' object"), arg, commit_type);
+
+   describe_commit(, );
+
+   puts(sb.buf);
 
if (!last_one)
clear_commit_marks(cmit, -1);
+
+   strbuf_release();
 }
 
 int cmd_describe(int argc, const char **argv, const char *prefix)
-- 
2.15.0.rc2.443.gfcc3b81c0a

[PATCHv2 1/7] list-objects.c: factor out traverse_trees_and_blobs

[Stefan Beller]

With traverse_trees_and_blobs factored out of the main traverse function,
the next patch can introduce an in-order revision walking with ease.

In the next patch we'll call `traverse_trees_and_blobs` from within the
loop walking the commits, such that we'll have one invocation of that
function per commit.  That is why we do not want to have memory allocations
in that function, such as we'd have if we were to use a strbuf locally.
Pass a strbuf from traverse_commit_list into the blob and tree traversing
function as a scratch pad that only needs to be allocated once.

Signed-off-by: Stefan Beller 
---
 list-objects.c | 50 +++---
 1 file changed, 31 insertions(+), 19 deletions(-)

diff --git a/list-objects.c b/list-objects.c
index b3931fa434..ef9dbe8f92 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -183,25 +183,15 @@ static void add_pending_tree(struct rev_info *revs, 
struct tree *tree)
add_pending_object(revs, >object, "");
 }
 
-void traverse_commit_list(struct rev_info *revs,
- show_commit_fn show_commit,
- show_object_fn show_object,
- void *data)
+static void traverse_trees_and_blobs(struct rev_info *revs,
+struct strbuf *base_path,
+show_object_fn show_object,
+void *data)
 {
int i;
-   struct commit *commit;
-   struct strbuf base;
 
-   strbuf_init(, PATH_MAX);
-   while ((commit = get_revision(revs)) != NULL) {
-   /*
-* an uninteresting boundary commit may not have its tree
-* parsed yet, but we are not going to show them anyway
-*/
-   if (commit->tree)
-   add_pending_tree(revs, commit->tree);
-   show_commit(commit, data);
-   }
+   assert(base_path->len == 0);
+
for (i = 0; i < revs->pending.nr; i++) {
struct object_array_entry *pending = revs->pending.objects + i;
struct object *obj = pending->item;
@@ -218,17 +208,39 @@ void traverse_commit_list(struct rev_info *revs,
path = "";
if (obj->type == OBJ_TREE) {
process_tree(revs, (struct tree *)obj, show_object,
-, path, data);
+base_path, path, data);
continue;
}
if (obj->type == OBJ_BLOB) {
process_blob(revs, (struct blob *)obj, show_object,
-, path, data);
+base_path, path, data);
continue;
}
die("unknown pending object %s (%s)",
oid_to_hex(>oid), name);
}
object_array_clear(>pending);
-   strbuf_release();
+}
+
+void traverse_commit_list(struct rev_info *revs,
+ show_commit_fn show_commit,
+ show_object_fn show_object,
+ void *data)
+{
+   struct commit *commit;
+   struct strbuf csp; /* callee's scratch pad */
+   strbuf_init(, PATH_MAX);
+
+   while ((commit = get_revision(revs)) != NULL) {
+   /*
+* an uninteresting boundary commit may not have its tree
+* parsed yet, but we are not going to show them anyway
+*/
+   if (commit->tree)
+   add_pending_tree(revs, commit->tree);
+   show_commit(commit, data);
+   }
+   traverse_trees_and_blobs(revs, , show_object, data);
+
+   strbuf_release();
 }
-- 
2.15.0.rc2.443.gfcc3b81c0a

[PATCHv2 2/7] revision.h: introduce blob/tree walking in order of the commits

[Stefan Beller]

The functionality to list tree objects in the order they were seen
while traversing the commits will be used in the next commit,
where we teach `git describe` to describe not only commits, but
trees and blobs, too.

Signed-off-by: Stefan Beller 
---
 Documentation/rev-list-options.txt |  5 +
 list-objects.c |  2 ++
 revision.c |  2 ++
 revision.h |  3 ++-
 t/t6100-rev-list-in-order.sh   | 46 ++
 5 files changed, 57 insertions(+), 1 deletion(-)
 create mode 100755 t/t6100-rev-list-in-order.sh

diff --git a/Documentation/rev-list-options.txt 
b/Documentation/rev-list-options.txt
index 13501e1556..9066e0c777 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -686,6 +686,11 @@ ifdef::git-rev-list[]
all object IDs which I need to download if I have the commit
object _bar_ but not _foo_''.
 
+--in-commit-order::
+   Print tree and blob ids in order of the commits. The tree
+   and blob ids are printed after they are first referenced
+   by a commit.
+
 --objects-edge::
Similar to `--objects`, but also print the IDs of excluded
commits prefixed with a ``-'' character.  This is used by
diff --git a/list-objects.c b/list-objects.c
index ef9dbe8f92..03438e5a8b 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -239,6 +239,8 @@ void traverse_commit_list(struct rev_info *revs,
if (commit->tree)
add_pending_tree(revs, commit->tree);
show_commit(commit, data);
+   if (revs->tree_blobs_in_commit_order)
+   traverse_trees_and_blobs(revs, , show_object, data);
}
traverse_trees_and_blobs(revs, , show_object, data);
 
diff --git a/revision.c b/revision.c
index d167223e69..9329d4ebbf 100644
--- a/revision.c
+++ b/revision.c
@@ -1845,6 +1845,8 @@ static int handle_revision_opt(struct rev_info *revs, int 
argc, const char **arg
revs->dense = 0;
} else if (!strcmp(arg, "--show-all")) {
revs->show_all = 1;
+   } else if (!strcmp(arg, "--in-commit-order")) {
+   revs->tree_blobs_in_commit_order = 1;
} else if (!strcmp(arg, "--remove-empty")) {
revs->remove_empty_trees = 1;
} else if (!strcmp(arg, "--merges")) {
diff --git a/revision.h b/revision.h
index 54761200ad..86985d68aa 100644
--- a/revision.h
+++ b/revision.h
@@ -121,7 +121,8 @@ struct rev_info {
bisect:1,
ancestry_path:1,
first_parent_only:1,
-   line_level_traverse:1;
+   line_level_traverse:1,
+   tree_blobs_in_commit_order:1;
 
/* Diff flags */
unsigned intdiff:1,
diff --git a/t/t6100-rev-list-in-order.sh b/t/t6100-rev-list-in-order.sh
new file mode 100755
index 00..651666979b
--- /dev/null
+++ b/t/t6100-rev-list-in-order.sh
@@ -0,0 +1,46 @@
+#!/bin/sh
+
+test_description='miscellaneous rev-list tests'
+
+. ./test-lib.sh
+
+
+test_expect_success 'setup' '
+   for x in one two three four
+   do
+   echo $x >$x &&
+   git add $x &&
+   git commit -m "add file $x"
+   done &&
+   for x in four three
+   do
+   git rm $x &&
+   git commit -m "remove $x"
+   done &&
+   git rev-list --in-commit-order --objects HEAD >actual.raw &&
+   cut -c 1-40 >actual expect.raw <<-\EOF &&
+   HEAD^{commit}
+   HEAD^{tree}
+   HEAD^{tree}:one
+   HEAD^{tree}:two
+   HEAD~1^{commit}
+   HEAD~1^{tree}
+   HEAD~1^{tree}:three
+   HEAD~2^{commit}
+   HEAD~2^{tree}
+   HEAD~2^{tree}:four
+   HEAD~3^{commit}
+   # HEAD~3^{tree} skipped
+   HEAD~4^{commit}
+   # HEAD~4^{tree} skipped
+   HEAD~5^{commit}
+   HEAD~5^{tree}
+   EOF
+   grep -v "#" >expect 

[PATCHv2 0/7] git describe blob

[Stefan Beller]

v2:

* other variable names in patch v1, the commit message explains the
  unusual strategy for the scratch pad variable, + assert
* less ugly test in p2
* typofix in p3 commit msg
* patch 4 (debug printing) unchanged, awaiting discussion to start/settle.
* rephrased the man page in p6.

Thanks,
Stefan

v1:
This is not an RFC any more, but a serious series.

Occasionally a user is given an object hash from a blob as an error message
or other output (e.g. [1]).

It would be useful to get a further description of such a blob, such as
the (commit, path) tuple where this blob was introduced.

This implements the answer in builtin/describe,
however the heuristics are weak. See patch 6 for details.

Any feedback welcome,

Thanks,
Stefan

Stefan Beller (7):
  list-objects.c: factor out traverse_trees_and_blobs
  revision.h: introduce blob/tree walking in order of the commits
  builtin/describe.c: rename `oid` to avoid variable shadowing
  builtin/describe.c: print debug statements earlier
  builtin/describe.c: factor out describe_commit
  builtin/describe.c: describe a blob
  t6120: fix typo in test name

 Documentation/git-describe.txt |  13 +++-
 Documentation/rev-list-options.txt |   5 ++
 builtin/describe.c | 125 -
 list-objects.c |  52 +--
 revision.c |   2 +
 revision.h |   3 +-
 t/t6100-rev-list-in-order.sh   |  46 ++
 t/t6120-describe.sh|  17 -
 8 files changed, 213 insertions(+), 50 deletions(-)
 create mode 100755 t/t6100-rev-list-in-order.sh

-- 
2.15.0.rc2.443.gfcc3b81c0a

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

[Stefan Beller]

On Mon, Oct 30, 2017 at 11:25 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
>> index c924c945ba..3d618b2445 100644
>> --- a/Documentation/git-describe.txt
>> +++ b/Documentation/git-describe.txt
>> @@ -3,7 +3,7 @@ git-describe(1)
>>
>>  NAME
>>  
>> -git-describe - Describe a commit using the most recent tag reachable from it
>> +git-describe - Describe a commit or blob using the most recent tag 
>> reachable from it
>
> If I am not mistaken, this series is about describing a blob as a
> tuple of a recent commit-ish and a path in the tree in it.  Blob
> never reaches anything, so "desribing blob using X reachable from
> it" is a mere nonsense.

Agreed, though the commitish used for the blob description may refer to a tag.

> The original is not great in that it ignores the "--contains" mode
> and referring to "tagged commit" merely as "tag" for brevity, but
> at least it made some sense.

Maybe

  "git-describe - Describe a blob or commit using graph relations"

though that sounds too generic, but it is accurate as all we do is
a heuristic for graph walk ways.


>> @@ -24,6 +24,16 @@ By default (without --all or --tags) `git describe` only 
>> shows
>>  annotated tags.  For more information about creating annotated tags
>>  see the -a and -s options to linkgit:git-tag[1].
>>
>> +If the given `` refers to a blob, it will be described
>
> Perhaps this step should update the SYNOPSIS so that the command
> takes not just commit-ish but a blob too.

That is a good idea, as the --contains would not work for blobs currently.

>  Given the difficulty in
> coming up with the single-liner description of what it does we saw
> above, I suspect that splitting SYNOPSIS out into two very distinct
> operating mode might make it easier to read.
>
> SYNOPSIS
> 
> [verse]
> 'git describe' [--all] [--tags] [--contains] [--abbrev=] 
> [...]
>+'git describe' [...] ...
>
> Then this additional paragraph can say "When describin a ",
> without using a (technically nonsense) phrase "if 
> refers to a blob", which is never true.
>

ok, do we know about 'blob-ish' as a term?

Re: [PATCH 4/7] builtin/describe.c: print debug statements earlier

[Stefan Beller]

On Tue, Oct 31, 2017 at 12:03 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> For debuggers aid we'd want to print debug statements early, so
>> introduce a new line in the debug output that describes the whole
>> function, and the change the next debug output to describe why we need
>> to search. Conveniently drop the arg from the second line; which will
>> be useful in a follow up commit, that refactors the describe function.
>>
>> Signed-off-by: Stefan Beller 
>> ---
>
> Adding an eary "entry" log may indeed be a good idea.  This is not a
> new issue, but I do not think it is a good trade-off to burden i18n
> teams to translate debug-only messages, though.

The original motivation here is to use the 'arg' argument so early in the
function that the next patch can omit passing in 'arg' as the lines
using 'arg' are not in the factored out function.

8713ab3079 (Improve git-describe performance by reducing
revision listing., 2007-01-13) added the --debug flag, and also
exposed it to the man page, so it is user visible, which is why we cannot
keep these messages untranslated.

>
>>  builtin/describe.c | 5 -
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/describe.c b/builtin/describe.c
>> index fd61f463cf..3136efde31 100644
>> --- a/builtin/describe.c
>> +++ b/builtin/describe.c
>> @@ -293,6 +293,9 @@ static void describe(const char *arg, int last_one)
>>   unsigned long seen_commits = 0;
>>   unsigned int unannotated_cnt = 0;
>>
>> + if (debug)
>> + fprintf(stderr, _("describe %s\n"), arg);
>> +
>>   if (get_oid(arg, ))
>>   die(_("Not a valid object name %s"), arg);
>>   cmit = lookup_commit_reference();
>> @@ -316,7 +319,7 @@ static void describe(const char *arg, int last_one)
>>   if (!max_candidates)
>>   die(_("no tag exactly matches '%s'"), 
>> oid_to_hex(>object.oid));
>>   if (debug)
>> - fprintf(stderr, _("searching to describe %s\n"), arg);
>> + fprintf(stderr, _("No exact match on refs or tags, searching 
>> to describe\n"));
>>
>>   if (!have_util) {
>>   struct hashmap_iter iter;

Re: [PATCH v3 4/4] fsmonitor: Delay updating state until after split index is merged

[Alex Vandiver]

On Tue, 31 Oct 2017, Junio C Hamano wrote:
> This makes local variable "int i;" in this function unused and gets
> compiler warning.

Apologies for leaving that detritus -- I saw you added a 'SQUASH??' commit
to deal with it, which LGTM.

On Tue, 31 Oct 2017, Johannes Schindelin wrote:
> ... to which end we introduced the DEVELOPER flag to catch these: if you
> call
> 
>   make DEVELOPER=1

Aha!  Thanks for the tip; I'll be sure to use that from now on.
 - Alex

Re: [RFC] protocol version 2

[Brandon Williams]

On 10/28, Philip Oakley wrote:
> From: "Brandon Williams" 
> Sent: Friday, October 20, 2017 6:18 PM
> >Objective
> >===
> >
> >Replace Git's current wire protocol with a simpler, less wasteful
> >protocol that can evolve over time.
> >
> 
> 
> 
> >Capability Advertisement
> >--
> >
> >A server which decides to communicate (based on a request from a client)
> >using protocol version 2, notifies the client by sending a version
> >string in its initial response followed by an advertisement of its
> >capabilities.  Each capability is a key with an optional value.  Clients
> >must ignore all unknown keys.
> 
> >   Semantics of unknown values are left to
> >the definition of each key.
> 
> This sounds odd. If the keys are known then their semantics are
> known. Or the keys are unknown and they and their values are
> ignored.
> 
> Maybe: Capability keys shall define their response to unknown key values.

I'll work to make the language a little clearer.

> 
> > Some capabilities will describe commands
> >which can be requested to be executed by the client.
> >
> 
> 
> >Ls-refs
> >-
> >
> >Ls-refs can be looked at as the equivalent of the current ls-remote as
> >it is a way to query a remote for the references that it has.  Unlike
> >the current ls-remote, the filtering of the output is done on the server
> >side by passing a number of parameters to the server-side command
> >instead of the filtering occurring on the client.
> >
> >Ls-ref takes in the following parameters:
> >
> > --head, --tags: Limit to only refs/heads or refs/tags
> > --refs: Do not show peeled tags or pseudorefs like HEAD
> > --symref: In addition to the object pointed by it, show the underlying
> >   ref pointed by it when showing a symbolic ref
> > : When specified, only references matching the given patterns
> >are displayed.
> 
> Does the --symref also the pseudorefs?
> 
> Isn't there a need somethimes to determine the ref that the remote's
> HEAD points to. This is an issue with the current clone and bundle
> code when there is a choice of refs/branches that could be the
> current HEAD ref and the wrong one is chosen, though this V2 change
> doesn't affect bundles.

Yeah, currently the resolution of HEAD is stuffed into the
capability line in v1.  The intention of this would be to allow for the
resolution of all symrefs (including HEAD).

> 
> >
> >The output of ls-refs is as follows:
> >
> >   output = (no-refs / list-of-refs)
> > *symref
> >*shallow
> >flush-pkt
> >
> >   no-refs = PKT-LINE(zero-id SP no-refs LF)
> >   list-of-refs = *ref
> >   ref = PKT-LINE((tip / peeled) LF)
> >   tip = obj-id SP refname
> >   peeled = obj-id SP refname "^{}"
> >
> >   symref = PKT-LINE("symref" SP symbolic-ref SP resolved-ref LF)
> >   shallow = PKT-LINE("shallow" SP obj-id LF)
> >
> >Fetch
> >---
> >
> >Fetch will need to be a modified version of the v1 fetch protocol.  Some
> >potential areas for improvement are: Ref-in-want, CDN offloading,
> >Fetch-options.
> >
> >Since we'll have an 'ls-ref' service we can eliminate the need of fetch
> >to perform a ref-advertisement, instead a client can run the 'ls-refs'
> >service first, in order to find out what refs the server has, and then
> >request those refs directly using the fetch service.
> >
> >//TODO Flush out the design
> >
> >Fetch-object
> >--
> >
> >This service could be used by partial clones in order to request missing
> >objects.
> >
> >//TODO Flush out the design
> >
> >Push
> >--
> >
> >Push will need to be a modified version of the v1 push protocol.  Some
> >potential areas for improvement are: Fix push-options, Negotiation for
> >force push.
> >
> >One change that will need to happen is to improve how `push-options` are
> >sent to the server (so that they aren't sent twice!!).  Also the
> >report-status needs to be better than it currently is in v1 so that
> >tools like gerrit can explain what it did with the ref-update the client
> >sent to it. Maybe have a push-rebase capability or command?
> >
> >//TODO Flush out the design
> >
> >Other Considerations
> >==
> >
> > * Move away from pkt-line framing?
> > * Have responses structured in well known formats (e.g. JSON)
> > * Eliminate initial round-trip using 'GIT_PROTOCOL' side-channel
> > * Additional commands in a partial clone world (e.g. log, grep)
> --
> Philip
> 

-- 
Brandon Williams

[PATCH v3 4/8] diff: remove touched flags

[Brandon Williams]

Now that the set of parallel touched flags are no longer being used,
remove them.

Signed-off-by: Brandon Williams 
---
 builtin/log.c | 2 --
 diff.h| 6 ++
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 82131751d..9c0815270 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -134,8 +134,6 @@ static void cmd_log_init_defaults(struct rev_info *rev)
 
if (default_date_mode)
parse_date_format(default_date_mode, >date_mode);
-
-   memset(>diffopt.touched_flags, 0, sizeof(struct diff_flags));
 }
 
 static void cmd_log_init_finish(int argc, const char **argv, const char 
*prefix,
diff --git a/diff.h b/diff.h
index d077d3c10..26a1cab87 100644
--- a/diff.h
+++ b/diff.h
@@ -107,9 +107,8 @@ static inline void diff_flags_or(struct diff_flags *a,
 }
 
 #define DIFF_OPT_TST(opts, flag)   ((opts)->flags.flag)
-#define DIFF_OPT_TOUCHED(opts, flag)   ((opts)->touched_flags.flag)
-#define DIFF_OPT_SET(opts, flag)   (((opts)->flags.flag = 
1),((opts)->touched_flags.flag = 1))
-#define DIFF_OPT_CLR(opts, flag)   (((opts)->flags.flag = 
0),((opts)->touched_flags.flag = 1))
+#define DIFF_OPT_SET(opts, flag)   ((opts)->flags.flag = 1)
+#define DIFF_OPT_CLR(opts, flag)   ((opts)->flags.flag = 0)
 
 #define DIFF_XDL_TST(opts, flag)((opts)->xdl_opts & XDF_##flag)
 #define DIFF_XDL_SET(opts, flag)((opts)->xdl_opts |= XDF_##flag)
@@ -138,7 +137,6 @@ struct diff_options {
const char *line_prefix;
size_t line_prefix_length;
struct diff_flags flags;
-   struct diff_flags touched_flags;
 
/* diff-filter bits */
unsigned int filter;
-- 
2.15.0.403.gc27cc4dac6-goog

[PATCH v3 0/8] convert diff flags to be stored in a struct

[Brandon Williams]

Changes in v3:
 * Now always pass struct diff_flags by reference and don't return the struct
   but rather modify the passed in struct.
 * Don't clear TEXTCONV_SET_VIA_CMDLINE when --no-textconv is passed
 * added additional patches (set out separately before) to remove the macros
   and change the struct members to lowercase

Brandon Williams (8):
  add, reset: use DIFF_OPT_SET macro to set a diff flag
  diff: convert flags to be stored in bitfields
  diff: add flag to indicate textconv was set via cmdline
  diff: remove touched flags
  diff: remove DIFF_OPT_TST macro
  diff: remove DIFF_OPT_SET macro
  diff: remove DIFF_OPT_CLR macro
  diff: make struct diff_flags members lowercase

 blame.c   |  16 ++---
 builtin/add.c |   4 +-
 builtin/am.c  |  10 +--
 builtin/blame.c   |  10 +--
 builtin/commit.c  |   7 +-
 builtin/diff.c|   8 +--
 builtin/fast-export.c |   2 +-
 builtin/log.c |  27 
 builtin/reset.c   |   2 +-
 builtin/rev-list.c|   2 +-
 combine-diff.c|  10 +--
 diff-lib.c|  30 +
 diff-no-index.c   |   8 +--
 diff.c| 175 +-
 diff.h|  88 ++---
 diffcore-pickaxe.c|   8 +--
 diffcore-rename.c |   6 +-
 log-tree.c|   2 +-
 merge-recursive.c |   4 +-
 notes-merge.c |   4 +-
 patch-ids.c   |   2 +-
 revision.c|  24 +++
 sequencer.c   |   5 +-
 submodule.c   |  16 ++---
 tree-diff.c   |  16 ++---
 wt-status.c   |  18 +++---
 26 files changed, 259 insertions(+), 245 deletions(-)

-- 
2.15.0.403.gc27cc4dac6-goog

[PATCH v3 1/8] add, reset: use DIFF_OPT_SET macro to set a diff flag

[Brandon Williams]

Instead of explicitly setting the 'DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG'
flag, use the 'DIFF_OPT_SET' macro.

Signed-off-by: Brandon Williams 
---
 builtin/add.c   | 2 +-
 builtin/reset.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index a648cf4c5..b70e8a779 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -116,7 +116,7 @@ int add_files_to_cache(const char *prefix,
rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
rev.diffopt.format_callback = update_callback;
rev.diffopt.format_callback_data = 
-   rev.diffopt.flags |= DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
+   DIFF_OPT_SET(, OVERRIDE_SUBMODULE_CONFIG);
rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
run_diff_files(, DIFF_RACY_IS_MODIFIED);
clear_pathspec(_data);
diff --git a/builtin/reset.c b/builtin/reset.c
index 9cd89b230..ea2fad5a0 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -166,7 +166,7 @@ static int read_from_tree(const struct pathspec *pathspec,
opt.output_format = DIFF_FORMAT_CALLBACK;
opt.format_callback = update_index_from_diff;
opt.format_callback_data = _to_add;
-   opt.flags |= DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
+   DIFF_OPT_SET(, OVERRIDE_SUBMODULE_CONFIG);
 
if (do_diff_cache(tree_oid, ))
return 1;
-- 
2.15.0.403.gc27cc4dac6-goog

[PATCH v3 5/8] diff: remove DIFF_OPT_TST macro

[Brandon Williams]

Remove the `DIFF_OPT_TST` macro and instead access the flags directly.
This conversion is done using the following semantic patch:

@@
expression E;
identifier fld;
@@
- DIFF_OPT_TST(, fld)
+ E.flags.fld

@@
type T;
T *ptr;
identifier fld;
@@
- DIFF_OPT_TST(ptr, fld)
+ ptr->flags.fld

Signed-off-by: Brandon Williams 
---
 blame.c|  8 +++---
 builtin/am.c   |  2 +-
 builtin/blame.c|  4 +--
 builtin/diff.c |  2 +-
 builtin/log.c  | 12 -
 builtin/rev-list.c |  2 +-
 combine-diff.c |  6 ++---
 diff-lib.c | 19 +++---
 diff-no-index.c|  2 +-
 diff.c | 76 +++---
 diff.h |  1 -
 diffcore-pickaxe.c |  8 +++---
 diffcore-rename.c  |  6 ++---
 log-tree.c |  2 +-
 revision.c |  4 +--
 submodule.c|  2 +-
 tree-diff.c| 12 -
 17 files changed, 84 insertions(+), 84 deletions(-)

diff --git a/blame.c b/blame.c
index f575e9cbf..7c019bc7c 100644
--- a/blame.c
+++ b/blame.c
@@ -209,7 +209,7 @@ static struct commit *fake_working_tree_commit(struct 
diff_options *opt,
 
switch (st.st_mode & S_IFMT) {
case S_IFREG:
-   if (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) &&
+   if (opt->flags.ALLOW_TEXTCONV &&
textconv_object(read_from, mode, _oid, 0, 
_ptr, _len))
strbuf_attach(, buf_ptr, buf_len, buf_len + 
1);
else if (strbuf_read_file(, read_from, st.st_size) 
!= st.st_size)
@@ -293,7 +293,7 @@ static void fill_origin_blob(struct diff_options *opt,
unsigned long file_size;
 
(*num_read_blob)++;
-   if (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) &&
+   if (opt->flags.ALLOW_TEXTCONV &&
textconv_object(o->path, o->mode, >blob_oid, 1, 
>ptr, _size))
;
else
@@ -1262,7 +1262,7 @@ static void find_copy_in_parent(struct blame_scoreboard 
*sb,
  >commit->tree->object.oid,
  "", _opts);
 
-   if (!DIFF_OPT_TST(_opts, FIND_COPIES_HARDER))
+   if (!diff_opts.flags.FIND_COPIES_HARDER)
diffcore_std(_opts);
 
do {
@@ -1825,7 +1825,7 @@ void setup_scoreboard(struct blame_scoreboard *sb, const 
char *path, struct blam
if (fill_blob_sha1_and_mode(o))
die(_("no such path %s in %s"), path, 
final_commit_name);
 
-   if (DIFF_OPT_TST(>revs->diffopt, ALLOW_TEXTCONV) &&
+   if (sb->revs->diffopt.flags.ALLOW_TEXTCONV &&
textconv_object(path, o->mode, >blob_oid, 1, (char **) 
>final_buf,
>final_buf_size))
;
diff --git a/builtin/am.c b/builtin/am.c
index d7513f537..fc54724cc 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1168,7 +1168,7 @@ static int index_has_changes(struct strbuf *sb)
strbuf_addstr(sb, diff_queued_diff.queue[i]->two->path);
}
diff_flush();
-   return DIFF_OPT_TST(, HAS_CHANGES) != 0;
+   return opt.flags.HAS_CHANGES != 0;
} else {
for (i = 0; sb && i < active_nr; i++) {
if (i)
diff --git a/builtin/blame.c b/builtin/blame.c
index 67adaef4d..a29574984 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -734,7 +734,7 @@ int cmd_blame(int argc, const char **argv, const char 
*prefix)
parse_revision_opt(, , options, blame_opt_usage);
}
 parse_done:
-   no_whole_file_rename = !DIFF_OPT_TST(, FOLLOW_RENAMES);
+   no_whole_file_rename = !revs.diffopt.flags.FOLLOW_RENAMES;
xdl_opts |= revs.diffopt.xdl_opts & XDF_INDENT_HEURISTIC;
DIFF_OPT_CLR(, FOLLOW_RENAMES);
argc = parse_options_end();
@@ -803,7 +803,7 @@ int cmd_blame(int argc, const char **argv, const char 
*prefix)
}
blame_date_width -= 1; /* strip the null */
 
-   if (DIFF_OPT_TST(, FIND_COPIES_HARDER))
+   if (revs.diffopt.flags.FIND_COPIES_HARDER)
opt |= (PICKAXE_BLAME_COPY | PICKAXE_BLAME_MOVE |
PICKAXE_BLAME_COPY_HARDER);
 
diff --git a/builtin/diff.c b/builtin/diff.c
index f5bbd4d75..8cbaf4538 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -44,7 +44,7 @@ static void stuff_change(struct diff_options *opt,
!oidcmp(old_oid, new_oid) && (old_mode == new_mode))
return;
 
-   if (DIFF_OPT_TST(opt, REVERSE_DIFF)) {
+   if (opt->flags.REVERSE_DIFF) {
SWAP(old_mode, new_mode);
SWAP(old_oid, new_oid);
SWAP(old_path, new_path);
diff --git a/builtin/log.c b/builtin/log.c
index 

[PATCH v3 2/8] diff: convert flags to be stored in bitfields

[Brandon Williams]

We cannot add many more flags to the diff machinery due to the
limitations of the number of flags that can be stored in a single
unsigned int.  In order to allow for more flags to be added to the diff
machinery in the future this patch converts the flags to be stored in
bitfields in 'struct diff_flags'.

Signed-off-by: Brandon Williams 
---
 builtin/commit.c |  7 +++--
 builtin/log.c|  3 +-
 diff-lib.c   |  7 +++--
 diff.c   |  2 +-
 diff.h   | 93 
 sequencer.c  |  5 +--
 6 files changed, 68 insertions(+), 49 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d75b3805e..960e7ac08 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -912,11 +912,12 @@ static int prepare_to_commit(const char *index_file, 
const char *prefix,
 * submodules which were manually staged, which would
 * be really confusing.
 */
-   int diff_flags = DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;
+   struct diff_flags flags = DIFF_FLAGS_INIT;
+   flags.OVERRIDE_SUBMODULE_CONFIG = 1;
if (ignore_submodule_arg &&
!strcmp(ignore_submodule_arg, "all"))
-   diff_flags |= DIFF_OPT_IGNORE_SUBMODULES;
-   commitable = index_differs_from(parent, diff_flags, 1);
+   flags.IGNORE_SUBMODULES = 1;
+   commitable = index_differs_from(parent, , 1);
}
}
strbuf_release(_ident);
diff --git a/builtin/log.c b/builtin/log.c
index d81a09051..dc28d43eb 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -134,7 +134,8 @@ static void cmd_log_init_defaults(struct rev_info *rev)
 
if (default_date_mode)
parse_date_format(default_date_mode, >date_mode);
-   rev->diffopt.touched_flags = 0;
+
+   memset(>diffopt.touched_flags, 0, sizeof(struct diff_flags));
 }
 
 static void cmd_log_init_finish(int argc, const char **argv, const char 
*prefix,
diff --git a/diff-lib.c b/diff-lib.c
index 4e0980caa..ed37f24c6 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -71,7 +71,7 @@ static int match_stat_with_submodule(struct diff_options 
*diffopt,
 {
int changed = ce_match_stat(ce, st, ce_option);
if (S_ISGITLINK(ce->ce_mode)) {
-   unsigned orig_flags = diffopt->flags;
+   struct diff_flags orig_flags = diffopt->flags;
if (!DIFF_OPT_TST(diffopt, OVERRIDE_SUBMODULE_CONFIG))
set_diffopt_flags_from_submodule_config(diffopt, 
ce->name);
if (DIFF_OPT_TST(diffopt, IGNORE_SUBMODULES))
@@ -534,7 +534,7 @@ int do_diff_cache(const struct object_id *tree_oid, struct 
diff_options *opt)
return 0;
 }
 
-int index_differs_from(const char *def, int diff_flags,
+int index_differs_from(const char *def, const struct diff_flags *flags,
   int ita_invisible_in_index)
 {
struct rev_info rev;
@@ -546,7 +546,8 @@ int index_differs_from(const char *def, int diff_flags,
setup_revisions(0, NULL, , );
DIFF_OPT_SET(, QUICK);
DIFF_OPT_SET(, EXIT_WITH_STATUS);
-   rev.diffopt.flags |= diff_flags;
+   if (flags)
+   diff_flags_or(, flags);
rev.diffopt.ita_invisible_in_index = ita_invisible_in_index;
run_diff_index(, 1);
object_array_clear();
diff --git a/diff.c b/diff.c
index 6fd288420..3ad9c9b31 100644
--- a/diff.c
+++ b/diff.c
@@ -5899,7 +5899,7 @@ int diff_can_quit_early(struct diff_options *opt)
 static int is_submodule_ignored(const char *path, struct diff_options *options)
 {
int ignored = 0;
-   unsigned orig_flags = options->flags;
+   struct diff_flags orig_flags = options->flags;
if (!DIFF_OPT_TST(options, OVERRIDE_SUBMODULE_CONFIG))
set_diffopt_flags_from_submodule_config(options, path);
if (DIFF_OPT_TST(options, IGNORE_SUBMODULES))
diff --git a/diff.h b/diff.h
index aca150ba2..e512cf44d 100644
--- a/diff.h
+++ b/diff.h
@@ -60,42 +60,56 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct 
diff_options *opt, void *data)
 
 #define DIFF_FORMAT_CALLBACK   0x1000
 
-#define DIFF_OPT_RECURSIVE   (1 <<  0)
-#define DIFF_OPT_TREE_IN_RECURSIVE   (1 <<  1)
-#define DIFF_OPT_BINARY  (1 <<  2)
-#define DIFF_OPT_TEXT(1 <<  3)
-#define DIFF_OPT_FULL_INDEX  (1 <<  4)
-#define DIFF_OPT_SILENT_ON_REMOVE(1 <<  5)
-#define DIFF_OPT_FIND_COPIES_HARDER  (1 <<  6)
-#define DIFF_OPT_FOLLOW_RENAMES  (1 <<  7)
-#define DIFF_OPT_RENAME_EMPTY(1 <<  8)
-/* (1 <<  9) unused */
-#define DIFF_OPT_HAS_CHANGES (1 << 10)
-#define DIFF_OPT_QUICK   (1 << 11)
-#define DIFF_OPT_NO_INDEX(1 << 12)
-#define DIFF_OPT_ALLOW_EXTERNAL  (1 << 13)
-#define 

[PATCH v3 3/8] diff: add flag to indicate textconv was set via cmdline

[Brandon Williams]

git-show is unique in that it wants to use textconv by default except
for when it is showing blobs.  When asked to show a blob, show doesn't
want to use textconv unless the user explicitly requested that it be
used by providing the command line flag '--textconv'.

Currently this is done by using a parallel set of 'touched' flags which
get set every time a particular flag is set or cleared.  In a future
patch we want to eliminate this parallel set of flags so instead of
relying on if the textconv flag has been touched, add a new flag
'TEXTCONV_SET_VIA_CMDLINE' which is only set if textconv is set to true
via the command line.

Signed-off-by: Brandon Williams 
---
 builtin/log.c | 2 +-
 diff.c| 5 +++--
 diff.h| 1 +
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index dc28d43eb..82131751d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -485,7 +485,7 @@ static int show_blob_object(const struct object_id *oid, 
struct rev_info *rev, c
unsigned long size;
 
fflush(rev->diffopt.file);
-   if (!DIFF_OPT_TOUCHED(>diffopt, ALLOW_TEXTCONV) ||
+   if (!DIFF_OPT_TST(>diffopt, TEXTCONV_SET_VIA_CMDLINE) ||
!DIFF_OPT_TST(>diffopt, ALLOW_TEXTCONV))
return stream_blob_to_fd(1, oid, NULL, 0);
 
diff --git a/diff.c b/diff.c
index 3ad9c9b31..11fccbd10 100644
--- a/diff.c
+++ b/diff.c
@@ -4762,9 +4762,10 @@ int diff_opt_parse(struct diff_options *options,
DIFF_OPT_SET(options, ALLOW_EXTERNAL);
else if (!strcmp(arg, "--no-ext-diff"))
DIFF_OPT_CLR(options, ALLOW_EXTERNAL);
-   else if (!strcmp(arg, "--textconv"))
+   else if (!strcmp(arg, "--textconv")) {
DIFF_OPT_SET(options, ALLOW_TEXTCONV);
-   else if (!strcmp(arg, "--no-textconv"))
+   DIFF_OPT_SET(options, TEXTCONV_SET_VIA_CMDLINE);
+   } else if (!strcmp(arg, "--no-textconv"))
DIFF_OPT_CLR(options, ALLOW_TEXTCONV);
else if (!strcmp(arg, "--ignore-submodules")) {
DIFF_OPT_SET(options, OVERRIDE_SUBMODULE_CONFIG);
diff --git a/diff.h b/diff.h
index e512cf44d..d077d3c10 100644
--- a/diff.h
+++ b/diff.h
@@ -83,6 +83,7 @@ struct diff_flags {
unsigned DIRSTAT_CUMULATIVE:1;
unsigned DIRSTAT_BY_FILE:1;
unsigned ALLOW_TEXTCONV:1;
+   unsigned TEXTCONV_SET_VIA_CMDLINE:1;
unsigned DIFF_FROM_CONTENTS:1;
unsigned DIRTY_SUBMODULES:1;
unsigned IGNORE_UNTRACKED_IN_SUBMODULES:1;
-- 
2.15.0.403.gc27cc4dac6-goog

[PATCH v3 8/8] diff: make struct diff_flags members lowercase

[Brandon Williams]

Now that the flags stored in struct diff_flags are being accessed
directly and not through macros, change all struct members from being
uppercase to lowercase.
This conversion is done using the following semantic patch:

@@
expression E;
@@
- E.RECURSIVE
+ E.recursive

@@
expression E;
@@
- E.TREE_IN_RECURSIVE
+ E.tree_in_recursive

@@
expression E;
@@
- E.BINARY
+ E.binary

@@
expression E;
@@
- E.TEXT
+ E.text

@@
expression E;
@@
- E.FULL_INDEX
+ E.full_index

@@
expression E;
@@
- E.SILENT_ON_REMOVE
+ E.silent_on_remove

@@
expression E;
@@
- E.FIND_COPIES_HARDER
+ E.find_copies_harder

@@
expression E;
@@
- E.FOLLOW_RENAMES
+ E.follow_renames

@@
expression E;
@@
- E.RENAME_EMPTY
+ E.rename_empty

@@
expression E;
@@
- E.HAS_CHANGES
+ E.has_changes

@@
expression E;
@@
- E.QUICK
+ E.quick

@@
expression E;
@@
- E.NO_INDEX
+ E.no_index

@@
expression E;
@@
- E.ALLOW_EXTERNAL
+ E.allow_external

@@
expression E;
@@
- E.EXIT_WITH_STATUS
+ E.exit_with_status

@@
expression E;
@@
- E.REVERSE_DIFF
+ E.reverse_diff

@@
expression E;
@@
- E.CHECK_FAILED
+ E.check_failed

@@
expression E;
@@
- E.RELATIVE_NAME
+ E.relative_name

@@
expression E;
@@
- E.IGNORE_SUBMODULES
+ E.ignore_submodules

@@
expression E;
@@
- E.DIRSTAT_CUMULATIVE
+ E.dirstat_cumulative

@@
expression E;
@@
- E.DIRSTAT_BY_FILE
+ E.dirstat_by_file

@@
expression E;
@@
- E.ALLOW_TEXTCONV
+ E.allow_textconv

@@
expression E;
@@
- E.TEXTCONV_SET_VIA_CMDLINE
+ E.textconv_set_via_cmdline

@@
expression E;
@@
- E.DIFF_FROM_CONTENTS
+ E.diff_from_contents

@@
expression E;
@@
- E.DIRTY_SUBMODULES
+ E.dirty_submodules

@@
expression E;
@@
- E.IGNORE_UNTRACKED_IN_SUBMODULES
+ E.ignore_untracked_in_submodules

@@
expression E;
@@
- E.IGNORE_DIRTY_SUBMODULES
+ E.ignore_dirty_submodules

@@
expression E;
@@
- E.OVERRIDE_SUBMODULE_CONFIG
+ E.override_submodule_config

@@
expression E;
@@
- E.DIRSTAT_BY_LINE
+ E.dirstat_by_line

@@
expression E;
@@
- E.FUNCCONTEXT
+ E.funccontext

@@
expression E;
@@
- E.PICKAXE_IGNORE_CASE
+ E.pickaxe_ignore_case

@@
expression E;
@@
- E.DEFAULT_FOLLOW_RENAMES
+ E.default_follow_renames

Signed-off-by: Brandon Williams 
---
 blame.c   |  16 ++---
 builtin/add.c |   4 +-
 builtin/am.c  |  10 +--
 builtin/blame.c   |  10 +--
 builtin/commit.c  |   4 +-
 builtin/diff.c|   8 +--
 builtin/fast-export.c |   2 +-
 builtin/log.c |  26 
 builtin/reset.c   |   2 +-
 builtin/rev-list.c|   2 +-
 combine-diff.c|  10 +--
 diff-lib.c|  22 +++
 diff-no-index.c   |   8 +--
 diff.c| 170 +-
 diff.h|  62 +-
 diffcore-pickaxe.c|   8 +--
 diffcore-rename.c |   6 +-
 log-tree.c|   2 +-
 merge-recursive.c |   4 +-
 notes-merge.c |   4 +-
 patch-ids.c   |   2 +-
 revision.c|  24 +++
 submodule.c   |  16 ++---
 tree-diff.c   |  16 ++---
 wt-status.c   |  18 +++---
 25 files changed, 228 insertions(+), 228 deletions(-)

diff --git a/blame.c b/blame.c
index dc9cc237b..28e03726f 100644
--- a/blame.c
+++ b/blame.c
@@ -209,7 +209,7 @@ static struct commit *fake_working_tree_commit(struct 
diff_options *opt,
 
switch (st.st_mode & S_IFMT) {
case S_IFREG:
-   if (opt->flags.ALLOW_TEXTCONV &&
+   if (opt->flags.allow_textconv &&
textconv_object(read_from, mode, _oid, 0, 
_ptr, _len))
strbuf_attach(, buf_ptr, buf_len, buf_len + 
1);
else if (strbuf_read_file(, read_from, st.st_size) 
!= st.st_size)

[PATCH v3 6/8] diff: remove DIFF_OPT_SET macro

[Brandon Williams]

Remove the `DIFF_OPT_SET` macro and instead set the flags directly.
This conversion is done using the following semantic patch:

@@
expression E;
identifier fld;
@@
- DIFF_OPT_SET(, fld)
+ E.flags.fld = 1

@@
type T;
T *ptr;
identifier fld;
@@
- DIFF_OPT_SET(ptr, fld)
+ ptr->flags.fld = 1

Signed-off-by: Brandon Williams 
---
 blame.c   |  8 +++
 builtin/add.c |  4 ++--
 builtin/am.c  |  8 +++
 builtin/blame.c   |  4 ++--
 builtin/diff.c|  6 ++---
 builtin/fast-export.c |  2 +-
 builtin/log.c | 14 +--
 builtin/reset.c   |  2 +-
 combine-diff.c|  2 +-
 diff-lib.c|  4 ++--
 diff-no-index.c   |  6 ++---
 diff.c| 66 +--
 diff.h|  1 -
 merge-recursive.c |  2 +-
 notes-merge.c |  4 ++--
 patch-ids.c   |  2 +-
 revision.c| 16 ++---
 submodule.c   |  8 +++
 tree-diff.c   |  4 ++--
 wt-status.c   | 18 +++---
 20 files changed, 90 insertions(+), 91 deletions(-)

diff --git a/blame.c b/blame.c
index 7c019bc7c..dc9cc237b 100644
--- a/blame.c
+++ b/blame.c
@@ -541,7 +541,7 @@ static struct blame_origin *find_origin(struct commit 
*parent,
 * same and diff-tree is fairly efficient about this.
 */
diff_setup(_opts);
-   DIFF_OPT_SET(_opts, RECURSIVE);
+   diff_opts.flags.RECURSIVE = 1;
diff_opts.detect_rename = 0;
diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT;
paths[0] = origin->path;
@@ -615,7 +615,7 @@ static struct blame_origin *find_rename(struct commit 
*parent,
int i;
 
diff_setup(_opts);
-   DIFF_OPT_SET(_opts, RECURSIVE);
+   diff_opts.flags.RECURSIVE = 1;
diff_opts.detect_rename = DIFF_DETECT_RENAME;
diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT;
diff_opts.single_follow = origin->path;
@@ -1238,7 +1238,7 @@ static void find_copy_in_parent(struct blame_scoreboard 
*sb,
return; /* nothing remains for this target */
 
diff_setup(_opts);
-   DIFF_OPT_SET(_opts, RECURSIVE);
+   diff_opts.flags.RECURSIVE = 1;
diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT;
 
diff_setup_done(_opts);
@@ -1253,7 +1253,7 @@ static void find_copy_in_parent(struct blame_scoreboard 
*sb,
if ((opt & PICKAXE_BLAME_COPY_HARDEST)
|| ((opt & PICKAXE_BLAME_COPY_HARDER)
&& (!porigin || strcmp(target->path, porigin->path
-   DIFF_OPT_SET(_opts, FIND_COPIES_HARDER);
+   diff_opts.flags.FIND_COPIES_HARDER = 1;
 
if (is_null_oid(>commit->object.oid))
do_diff_cache(>tree->object.oid, _opts);
diff --git a/builtin/add.c b/builtin/add.c
index b70e8a779..e1d83b69a 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -116,7 +116,7 @@ int add_files_to_cache(const char *prefix,
rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
rev.diffopt.format_callback = update_callback;
rev.diffopt.format_callback_data = 
-   DIFF_OPT_SET(, OVERRIDE_SUBMODULE_CONFIG);
+   rev.diffopt.flags.OVERRIDE_SUBMODULE_CONFIG = 1;
rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
run_diff_files(, DIFF_RACY_IS_MODIFIED);
clear_pathspec(_data);
@@ -218,7 +218,7 @@ static int edit_patch(int argc, const char **argv, const 
char *prefix)
argc = setup_revisions(argc, argv, , NULL);
rev.diffopt.output_format = DIFF_FORMAT_PATCH;
rev.diffopt.use_color = 0;
-   DIFF_OPT_SET(, IGNORE_DIRTY_SUBMODULES);
+   rev.diffopt.flags.IGNORE_DIRTY_SUBMODULES = 1;
out = open(file, O_CREAT | O_WRONLY, 0666);
if (out < 0)
die(_("Could not open '%s' for writing."), file);
diff --git a/builtin/am.c b/builtin/am.c
index fc54724cc..015425a0f 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1157,9 +1157,9 @@ static int index_has_changes(struct strbuf *sb)
struct diff_options opt;
 
diff_setup();
-   DIFF_OPT_SET(, EXIT_WITH_STATUS);
+   opt.flags.EXIT_WITH_STATUS = 1;
if (!sb)
-   DIFF_OPT_SET(, QUICK);
+   opt.flags.QUICK = 1;
do_diff_cache(, );
diffcore_std();
for (i = 0; sb && i < diff_queued_diff.nr; i++) {
@@ -1409,8 +1409,8 @@ static void write_commit_patch(const struct am_state 
*state, struct commit *comm
rev_info.show_root_diff = 1;
rev_info.diffopt.output_format = DIFF_FORMAT_PATCH;
rev_info.no_commit_id = 1;
-   DIFF_OPT_SET(_info.diffopt, BINARY);
-   DIFF_OPT_SET(_info.diffopt, FULL_INDEX);
+   rev_info.diffopt.flags.BINARY = 1;
+   rev_info.diffopt.flags.FULL_INDEX 

[PATCH v3 7/8] diff: remove DIFF_OPT_CLR macro

[Brandon Williams]

Remove the `DIFF_OPT_SET` macro and instead set the flags directly.
This conversion is done using the following semantic patch:

@@
expression E;
identifier fld;
@@
- DIFF_OPT_CLR(, fld)
+ E.flags.fld = 0

@@
type T;
T *ptr;
identifier fld;
@@
- DIFF_OPT_CLR(ptr, fld)
+ ptr->flags.fld = 0

Signed-off-by: Brandon Williams 
---
 builtin/blame.c   |  2 +-
 combine-diff.c|  2 +-
 diff.c| 28 ++--
 diff.h|  2 --
 merge-recursive.c |  2 +-
 revision.c|  4 ++--
 submodule.c   |  6 +++---
 7 files changed, 22 insertions(+), 24 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 76994aa64..79db9e849 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -736,7 +736,7 @@ int cmd_blame(int argc, const char **argv, const char 
*prefix)
 parse_done:
no_whole_file_rename = !revs.diffopt.flags.FOLLOW_RENAMES;
xdl_opts |= revs.diffopt.xdl_opts & XDF_INDENT_HEURISTIC;
-   DIFF_OPT_CLR(, FOLLOW_RENAMES);
+   revs.diffopt.flags.FOLLOW_RENAMES = 0;
argc = parse_options_end();
 
if (incremental || (output_option & OUTPUT_PORCELAIN)) {
diff --git a/combine-diff.c b/combine-diff.c
index 204b0dfce..5a3a8b49b 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1414,7 +1414,7 @@ void diff_tree_combined(const struct object_id *oid,
diffopts = *opt;
copy_pathspec(, >pathspec);
diffopts.flags.RECURSIVE = 1;
-   DIFF_OPT_CLR(, ALLOW_EXTERNAL);
+   diffopts.flags.ALLOW_EXTERNAL = 0;
 
/* find set of paths that everybody touches
 *
diff --git a/diff.c b/diff.c
index 6dea186d8..e5f9d3078 100644
--- a/diff.c
+++ b/diff.c
@@ -124,16 +124,16 @@ static int parse_dirstat_params(struct diff_options 
*options, const char *params
for (i = 0; i < params.nr; i++) {
const char *p = params.items[i].string;
if (!strcmp(p, "changes")) {
-   DIFF_OPT_CLR(options, DIRSTAT_BY_LINE);
-   DIFF_OPT_CLR(options, DIRSTAT_BY_FILE);
+   options->flags.DIRSTAT_BY_LINE = 0;
+   options->flags.DIRSTAT_BY_FILE = 0;
} else if (!strcmp(p, "lines")) {
options->flags.DIRSTAT_BY_LINE = 1;
-   DIFF_OPT_CLR(options, DIRSTAT_BY_FILE);
+   options->flags.DIRSTAT_BY_FILE = 0;
} else if (!strcmp(p, "files")) {
-   DIFF_OPT_CLR(options, DIRSTAT_BY_LINE);
+   options->flags.DIRSTAT_BY_LINE = 0;
options->flags.DIRSTAT_BY_FILE = 1;
} else if (!strcmp(p, "noncumulative")) {
-   DIFF_OPT_CLR(options, DIRSTAT_CUMULATIVE);
+   options->flags.DIRSTAT_CUMULATIVE = 0;
} else if (!strcmp(p, "cumulative")) {
options->flags.DIRSTAT_CUMULATIVE = 1;
} else if (isdigit(*p)) {
@@ -4205,7 +4205,7 @@ void diff_setup_done(struct diff_options *options)
DIFF_XDL_TST(options, IGNORE_WHITESPACE_AT_EOL))
options->flags.DIFF_FROM_CONTENTS = 1;
else
-   DIFF_OPT_CLR(options, DIFF_FROM_CONTENTS);
+   options->flags.DIFF_FROM_CONTENTS = 0;
 
if (options->flags.FIND_COPIES_HARDER)
options->detect_rename = DIFF_DETECT_COPY;
@@ -4640,7 +4640,7 @@ int diff_opt_parse(struct diff_options *options,
else if (!strcmp(arg, "--rename-empty"))
options->flags.RENAME_EMPTY = 1;
else if (!strcmp(arg, "--no-rename-empty"))
-   DIFF_OPT_CLR(options, RENAME_EMPTY);
+   options->flags.RENAME_EMPTY = 0;
else if (!strcmp(arg, "--relative"))
options->flags.RELATIVE_NAME = 1;
else if (skip_prefix(arg, "--relative=", )) {
@@ -4697,8 +4697,8 @@ int diff_opt_parse(struct diff_options *options,
else if (!strcmp(arg, "--follow"))
options->flags.FOLLOW_RENAMES = 1;
else if (!strcmp(arg, "--no-follow")) {
-   DIFF_OPT_CLR(options, FOLLOW_RENAMES);
-   DIFF_OPT_CLR(options, DEFAULT_FOLLOW_RENAMES);
+   options->flags.FOLLOW_RENAMES = 0;
+   options->flags.DEFAULT_FOLLOW_RENAMES = 0;
} else if (!strcmp(arg, "--color"))
options->use_color = 1;
else if (skip_prefix(arg, "--color=", )) {
@@ -4761,12 +4761,12 @@ int diff_opt_parse(struct diff_options *options,
else if (!strcmp(arg, "--ext-diff"))
options->flags.ALLOW_EXTERNAL = 1;
else if (!strcmp(arg, "--no-ext-diff"))
-   DIFF_OPT_CLR(options, ALLOW_EXTERNAL);
+   options->flags.ALLOW_EXTERNAL = 0;
else if (!strcmp(arg, "--textconv")) {

Re: [PATCH 2/7] revision.h: introduce blob/tree walking in order of the commits

[Stefan Beller]

On Mon, Oct 30, 2017 at 11:57 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> diff --git a/list-objects.c b/list-objects.c
>> index bf46f80dff..5e114c9a8a 100644
>> --- a/list-objects.c
>> +++ b/list-objects.c
>> @@ -237,6 +237,8 @@ void traverse_commit_list(struct rev_info *revs,
>>   if (commit->tree)
>>   add_pending_tree(revs, commit->tree);
>>   show_commit(commit, data);
>> + if (revs->tree_blobs_in_commit_order)
>> + traverse_trees_and_blobs(revs, _path, 
>> show_object, data);
>>   }
>>   traverse_trees_and_blobs(revs, _path, show_object, data);
>
> This one is interesting.  While we walk the ancestry chain, we may
> add the tree for each commit that we discover to the "pending.  We
> used to sweep the pending array at the end after we run out of the
> commits, but now we do the sweeping as we process each commit.
>
> Would this make the last call to traverse_trees_and_blobs() always a
> no-op?

That is my understanding of the code, the important part of the previous patch
was the move of 'object_array_clear(>pending);' into the
`traverse_trees_and_blobs` function.

>  I am not suggesting to add a new conditional in front of it;
> I am just asking for curiosity's sake.

Thanks for being clear on that. I have the same taste for this.

(Though for a split second I wondered if we can pull some esoteric trick
to skip this, but I could not find any that is sane as well as readable.)

>> +test_expect_success 'setup' '
>> + for x in one two three four
>> + do
>> + echo $x >$x &&
>> + git add $x &&
>> + git commit -m "add file $x"
>> + done &&
>> + for x in four three
>> + do
>> + git rm $x
>> + git commit -m "remove $x"
>> + done &&
>
> There is break in &&-chain in the second loop, but in any case, for
> loops and &&-chains do not mix very well unless done carefully.
> Imagine what happens if "git commit" in the first loop failed when
> x is two; it won't stop and keep going to create three and four and
> nobody would noice any error.
>

I'll fix that.


> Even though we do not have --stdin for rev-parse, you can still do:
>
> git cat-file --batch-check='%(objectname)' >expect <<-\EOF &&
> HEAD^{commit}
> HEAD^{tree}
> HEAD:one
> HEAD:two
> ...
> EOF

sounds better than the current implementation.

Re: [PATCH 0/3] mingw: introduce a way to avoid std handle inheritance

[Jonathan Nieder]

Hi,

Johannes Schindelin wrote:
> On Mon, 30 Oct 2017, Jonathan Nieder wrote:

>> Can this rationale go in the commit messages?
>
> I thought I had done exactly that in 1/3...

Okay, I'll be more specific.  This cover letter includes some
information about the rationale and motivation for the series.  That's
great: it makes reading the patches easier.  But TBH I'd rather that
it hadn't included that information at all, since if it said "see
patch 1/3 for rationale" then I could save the trouble of reading the
same information twice.

And unfortunately much of the relevant information is not repeated
there.  The cover letter mentions:

- that Visual Studio is a motivating example
- that this is conceptually similar to Unix sockets
- that those do not need to be marked as inheritable, as the process
  can simply open the named pipe. No global flags. No problems.
- that this has already seem some testing in Git for Windows (i.e.
  analagous information to what a Tested-by footer would say)

It is also just more readable than patch 1/3's commit message.  That's
to be expected, since it was written later.  My second draft of
something is often clearer than the first draft.

Thanks and hope that helps,
Jonathan

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

[Paolo Ciarrocchi]

On Tue, Oct 31, 2017 at 6:21 PM, Johannes Schindelin
 wrote:
> Hi Paolo,
>
> On Mon, 30 Oct 2017, Paolo Ciarrocchi wrote:
>
>> On Mon, Oct 30, 2017 at 1:35 PM, Johannes Schindelin
>>  wrote:
>> >
>> > On Mon, 30 Oct 2017, Junio C Hamano wrote:
>> >
>> >> "brian m. carlson"  writes:
>> >>
>> >> Thanks.  I personally prefer the plain-text original, but I do
>> >> understand the need to have a version with ids that you can tell
>> >> others to visit in their browsers.  Assuming that this goes in the
>> >> right direction, here are a few comments.
>> >
>> > If you want to go into the direction of the web, AsciiDoc is actually the
>> > wrong choice IMO, and Markdown would be the right choice. Basically
>> > everybody on the web is either supporting Markdown or being asked by users
>> > to do so.
>> >
>> > Assuming that *that* is something we want to pursue, I would also suggest
>> > to move the man pages away from AsciiDoc to Markdown (using e.g.
>> > [ronn](https://rtomayko.github.io/ronn/ronn.1.html)).
>>
>> Nitpick, the right URL is: https://rtomayko.github.io/ronn/ronn.1.html
>> You put an extra ')' at the end and therefore I've got a scaring 404 error 
>> :-)
>
> The first closing parenthesis closes the link associated with the label
> `ronn`, the second closing parenthesis closes the remark I started in the
> previous line (beginning with the word "using").


You are right. I've got confused by the fact that gmail fails to
properly handle that link.
Sorry for the noise.

Regards,
-- 
Paolo

Re: Bug/Wish: bash completion for git pull --rebase doesn't include --autostash

[Stefan Beller]

On Tue, Oct 31, 2017 at 8:21 AM, Albert Astals Cid
 wrote:
> git pull --rebase --autostash
>
> is a valid command but the --autostash autocompletion is not suggested after
> typing
>
> git pul --reb
> Would be great if that could be added.
>
> Thanks :)
>
> Albert
>
> P.S: I'm not subscribed CC me if need me to test something

Maybe you can propose a patch for this?

Rough steps to success:

  git clone git://git.kernel.org/pub/scm/git/git.git/
  cd git
  $EDIT contrib/completion/git-completion.bash
   (look for _git_pull)
  git commit -m "my first commit to git"
  (This is paraphrased, see Documentation/SubmittingPatches
   for a better idea how to craft commit messages)
  git format-patch HEAD^
  git send-email 0001-xxx.patch



>
> --
> Albert Astals Cid | albert.astals@kdab.com | Software Engineer
> Klarälvdalens Datakonsult AB, a KDAB Group company
> Tel: Sweden (HQ) +46-563-540090, USA +1-866-777-KDAB(5322)
> KDAB - The Qt, C++ and OpenGL Experts

Re: [PATCH 3.5/4] diff: set TEXTCONV_VIA_CMDLINE only when it is set to true

[Brandon Williams]

On 10/31, Junio C Hamano wrote:
> Change the meaning of the bit to "the user explicitly set the
> allow-textconv bit to true from the command line".
> 
> The "touched" mechanism in the old code meant to express "the user
> explicitly set the allow-textconv bit to something from the command
> line" and recorded that fact upon "--no-textconv", too, by setting
> the corresponding touched bit.  The code in the previous step to
> clear the bit did not make much sense.
> 
> Again, this may want be squashed into the previous step, but its log
> message needs to be adjusted somewhat (e.g. "s/is requested via/is
> set to true via/").

I don't have any opinions on this, but I agree that if we want a more
true conversion then we would wanted to squash this in, which I'll do
and update the log message.

> 
> Signed-off-by: Junio C Hamano 
> ---
>  diff.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/diff.c b/diff.c
> index 8b700b1bd2..11fccbd107 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4765,10 +4765,9 @@ int diff_opt_parse(struct diff_options *options,
>   else if (!strcmp(arg, "--textconv")) {
>   DIFF_OPT_SET(options, ALLOW_TEXTCONV);
>   DIFF_OPT_SET(options, TEXTCONV_SET_VIA_CMDLINE);
> - } else if (!strcmp(arg, "--no-textconv")) {
> + } else if (!strcmp(arg, "--no-textconv"))
>   DIFF_OPT_CLR(options, ALLOW_TEXTCONV);
> - DIFF_OPT_CLR(options, TEXTCONV_SET_VIA_CMDLINE);
> - } else if (!strcmp(arg, "--ignore-submodules")) {
> + else if (!strcmp(arg, "--ignore-submodules")) {
>   DIFF_OPT_SET(options, OVERRIDE_SUBMODULE_CONFIG);
>   handle_ignore_submodules_arg(options, "all");
>   } else if (skip_prefix(arg, "--ignore-submodules=", )) {
> -- 
> 2.15.0-224-g5109123e6a
> 

-- 
Brandon Williams

Re: [PATCH 2.5/4] diff: avoid returning a struct by value from diff_flags_or()

[Brandon Williams]

On 10/31, Junio C Hamano wrote:
> That is more in line with the design decision made in the previous
> step to pass struct by reference.
> 
> We may want to squash this into the previous patch eventually.

Looks good to me.  I was on the fence with what to do since I was
already moving to pass by reference.  This is probably the better move
so I can squash this one into the previous.

> 
> Signed-off-by: Junio C Hamano 
> ---
> 
>  * I am OK either way as long as things are consistent; as you took
>time to change the code to pass the struct by reference, let's
>unify the API in that direction.
> 
>  diff-lib.c |  2 +-
>  diff.h | 12 
>  2 files changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/diff-lib.c b/diff-lib.c
> index 6c1c05c5b0..ed37f24c68 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -547,7 +547,7 @@ int index_differs_from(const char *def, const struct 
> diff_flags *flags,
>   DIFF_OPT_SET(, QUICK);
>   DIFF_OPT_SET(, EXIT_WITH_STATUS);
>   if (flags)
> - rev.diffopt.flags = diff_flags_or(, flags);
> + diff_flags_or(, flags);
>   rev.diffopt.ita_invisible_in_index = ita_invisible_in_index;
>   run_diff_index(, 1);
>   object_array_clear();
> diff --git a/diff.h b/diff.h
> index 47e6d43cbc..e512cf44d0 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -94,19 +94,15 @@ struct diff_flags {
>   unsigned DEFAULT_FOLLOW_RENAMES:1;
>  };
>  
> -static inline struct diff_flags diff_flags_or(const struct diff_flags *a,
> -   const struct diff_flags *b)
> +static inline void diff_flags_or(struct diff_flags *a,
> +  const struct diff_flags *b)
>  {
> - struct diff_flags out;
>   char *tmp_a = (char *)a;
> - char *tmp_b = (char *)b;
> - char *tmp_out = (char *)
> + const char *tmp_b = (const char *)b;
>   int i;
>  
>   for (i = 0; i < sizeof(struct diff_flags); i++)
> - tmp_out[i] = tmp_a[i] | tmp_b[i];
> -
> - return out;
> + tmp_a[i] |= tmp_b[i];
>  }
>  
>  #define DIFF_OPT_TST(opts, flag) ((opts)->flags.flag)
> -- 
> 2.15.0-224-g5109123e6a
> 

-- 
Brandon Williams

Re: [PATCH] t5580: add Cygwin support

[Johannes Schindelin]

Hi Adam,

On Tue, 31 Oct 2017, Adam Dinwoodie wrote:

> t5580 tests that specifying Windows UNC paths works with Git.  Cygwin
> supports UNC paths, albeit only using forward slashes, not backslashes,
> so run the compatible tests on Cygwin as well as MinGW.
> 
> The only complication is Cygwin's `pwd`, which returns a *nix-style
> path, and that's not suitable for calculating the UNC path to the
> current directory.  Instead use Cygwin's `cygpath` utility to get the
> Windows-style path.

Looks good!

Thank you,
Dscho

Re: [PATCH v3 4/4] fsmonitor: Delay updating state until after split index is merged

[Johannes Schindelin]

Hi,

On Tue, 31 Oct 2017, Junio C Hamano wrote:

> Alex Vandiver  writes:
> 
> > diff --git a/fsmonitor.c b/fsmonitor.c
> > index 4ea44dcc6..417759224 100644
> > --- a/fsmonitor.c
> > +++ b/fsmonitor.c
> > @@ -49,20 +49,7 @@ int read_fsmonitor_extension(struct index_state *istate, 
> > const void *data,
> > ewah_free(fsmonitor_dirty);
> > return error("failed to parse ewah bitmap reading fsmonitor 
> > index extension");
> > }
> > -
> > -   if (git_config_get_fsmonitor()) {
> > -   /* Mark all entries valid */
> > -   for (i = 0; i < istate->cache_nr; i++)
> > -   istate->cache[i]->ce_flags |= CE_FSMONITOR_VALID;
> > -
> > -   /* Mark all previously saved entries as dirty */
> > -   ewah_each_bit(fsmonitor_dirty, fsmonitor_ewah_callback, istate);
> > -
> > -   /* Now mark the untracked cache for fsmonitor usage */
> > -   if (istate->untracked)
> > -   istate->untracked->use_fsmonitor = 1;
> > -   }
> > -   ewah_free(fsmonitor_dirty);
> > +   istate->fsmonitor_dirty = fsmonitor_dirty;
> 
> This makes local variable "int i;" in this function unused and gets
> compiler warning.

... to which end we introduced the DEVELOPER flag to catch these: if you
call

make DEVELOPER=1

and compile with GCC or Clang, it will elevate such warnings to errors,
and we highly encourage contributors to build their patched source code
with said flag.

Thanks,
Johannes

Is it possible to convert a Json file to xml file with Git

[Eyjolfur Eyjolfsson]

Hi

I have a question.
Is it possible to convert a Json file to XML with Git

Best regards

Eyjolfur Eyjolfsson

(e) eyjolfureyjolfs...@tprg.com
(w) tpretailgroup.com

-- 
This email and any files transmitted with it are confidential and intended 
for the sole use of the individual or entity to whom they are addressed. 
 Any unauthorised dissemination or copying of this email or its attachments 
or disclosure of any information contained in them is strictly prohibited. 
 If you have received the email in error, please notify the sender by email 
immediately and delete it from your system.  The content of the email does 
not necessarily represent Theo Paphitis Retail Group and associated 
companies and any views or opinions presented are solely those of the 
author.  Whilst we check communications we send for virus infection, you 
should check this email and any attachments to it for viruses as we accept 
no responsibility for any loss or damage caused by any virus transmitted by 
this email.  Email transmission cannot be guaranteed secure or error-free.

Theo Paphitis Retail Group is the collective name for Ryman Group Limited, 
registered in England and Wales, Company Number 02714395, VAT Number 
672523729,  Registered Office: Ryman House, Savoy Road, Crewe, Cheshire, 
CW1 6NA; Ryman Limited, registered in England and Wales, Company Number 
3007166, VAT Number 672523729,  Registered Office: Ryman House, Savoy Road, 
Crewe, Cheshire, CW1 6NA;  Robert Dyas Holdings Limited, registered in 
England and Wales, Company Number 4041884; VAT number 742720153, Registered 
Office:  1 St George’s Road, Wimbledon, London, SW19 4DR;  Boux Avenue 
Limited, registered in England and Wales, Company Number 7191520, VAT 
Number 125504638,  Registered Office: 1 St George’s Road, Wimbledon, 
London, SW19 4DR;  Boux Avenue International Limited, registered in England 
and Wales, Company Number 8047333, VAT Number 125504638, Registered office: 
1 St George’s Road, Wimbledon, London, SW19 4DR; and London Graphic Centre 
Limited, registered in England and Wales, Company Number 6062021,  VAT 
Number 251820524, Registered Office:  Ryman House, Savoy Road, Crewe, 
Cheshire, CW1 6NA.

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

[Johannes Schindelin]

Hi Paolo,

On Mon, 30 Oct 2017, Paolo Ciarrocchi wrote:

> On Mon, Oct 30, 2017 at 1:35 PM, Johannes Schindelin
>  wrote:
> >
> > On Mon, 30 Oct 2017, Junio C Hamano wrote:
> >
> >> "brian m. carlson"  writes:
> >>
> >> Thanks.  I personally prefer the plain-text original, but I do
> >> understand the need to have a version with ids that you can tell
> >> others to visit in their browsers.  Assuming that this goes in the
> >> right direction, here are a few comments.
> >
> > If you want to go into the direction of the web, AsciiDoc is actually the
> > wrong choice IMO, and Markdown would be the right choice. Basically
> > everybody on the web is either supporting Markdown or being asked by users
> > to do so.
> >
> > Assuming that *that* is something we want to pursue, I would also suggest
> > to move the man pages away from AsciiDoc to Markdown (using e.g.
> > [ronn](https://rtomayko.github.io/ronn/ronn.1.html)).
> 
> Nitpick, the right URL is: https://rtomayko.github.io/ronn/ronn.1.html
> You put an extra ')' at the end and therefore I've got a scaring 404 error :-)

The first closing parenthesis closes the link associated with the label
`ronn`, the second closing parenthesis closes the remark I started in the
previous line (beginning with the word "using").

Ciao,
Johannes

Re: What's cooking in git.git (Oct 2017, #07; Mon, 30)

[Johannes Schindelin]

Hi Junio,

On Tue, 31 Oct 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > On Mon, 30 Oct 2017, Junio C Hamano wrote:
> >
> >> * jc/branch-name-sanity (2017-10-14) 3 commits
> >>   (merged to 'next' on 2017-10-16 at 174646d1c3)
> >>  + branch: forbid refs/heads/HEAD
> >>  + branch: split validate_new_branchname() into two
> >>  + branch: streamline "attr_only" handling in validate_new_branchname()
> >> 
> >>  "git branch" and "git checkout -b" are now forbidden from creating
> >>  a branch whose name is "HEAD".
> >
> > Question: should we respect core.ignoreCase and if it is true, compare
> > case-insensitively? Or should we just keep the comparison
> > case-sensitively, in preparation for a (hopefully near) refs backend that
> > does not inherit filesystems' case-insensitivity?
> 
> While I do think it would be good if the system as a whole somewhere
> we had a safety to say "We do not allow hEaD as branch name as you
> are using the files-backend as your reference storage on a case
> insensitive filesystem", I do not think it is a good idea to do so
> in the layer the above patches touch.  Once a more capable ref
> backend comes (Shawn's reftable, anybody???), platforms with case
> insensitive filesystems can allow refs/heads/hEaD as a branch whose
> name is hEaD that is different from another branch whose name is
> hEAD just fine; having the "we are on icase system, so reject" check
> at the layer would mean we need to remember we have such a check at
> a wrong layer and revert it when such an improvement happens.

I am glad you covered that concern, thanks!
Dscho

Re: [PATCH 1/2] t0302: check helper can handle empty credentials

[Johannes Schindelin]

Hi Peff,

On Mon, 30 Oct 2017, Jeff King wrote:

> On Mon, Oct 30, 2017 at 06:20:12PM +0100, Johannes Schindelin wrote:
> 
> > Subject: Re: [PATCH 1/2] t0302: check helper can handle empty credentials
> 
> I guess we really care about t0303 here (which tests external helpers).
> This patch adds the test to lib-credential, so it hits the "cache" and
> "store" helpers, too. Which seems to pass, so I guess that's OK (I have
> to admit that as the author of those tools, I wasn't sure how they'd
> react).
> 
> > Make sure the helper does not crash when blank username and password is
> > provided. If the helper can save such credentials, it should be able to
> > read them back.
> 
> I worry that some third-party helpers might not be able to represent
> this case and would fail the test. This has been around for years no
> Windows, but probably hasn't ever been run with osxkeychain or
> libsecret. I'd be OK with taking this as-is, though, and waiting to see
> if anybody complains. At that point we'll know if the right solution is
> enhancing that helper, or providing a way to optionally skip this test.

Okay. If you change your mind, please let me know, I would try to set
aside some time to adjust the patch in that event.

> (Though I have no idea if anybody actually runs t0303 against
> custom-built helpers in the first place. The process is pretty manual
> for now, though the Makefiles in contrib/credential could probably at
> least provide a "make test").

Right... I am not aware of any attempts to run those tests against Git
Credential Manager for Windows, for example...

Ciao,
Dscho

Re: [PATCH 0/3] mingw: introduce a way to avoid std handle inheritance

[Johannes Schindelin]

Hi Jonathan,

On Mon, 30 Oct 2017, Jonathan Nieder wrote:

> Johannes Schindelin wrote:
> 
> > Particularly when calling Git from applications, such as Visual Studio,
> > it is important that stdin/stdout/stderr are closed properly. However,
> > when spawning processes on Windows, those handles must be marked as
> > inheritable if we want to use them, but that flag is a global flag and
> > may very well be used by other spawned processes which then do not know
> > to close those handles.
> >
> > As a workaround, introduce handling for the environment variables
> > GIT_REDIRECT_STD* to read/write from/to named pipes instead
> > (conceptually similar to Unix sockets, for you Linux folks). These do
> > not need to be marked as inheritable, as the process can simply open the
> > named pipe. No global flags. No problems.
> >
> > This feature was introduced as an experimental feature into Git for
> > Windows v2.11.0(2) and has been tested ever since. I feel it is
> > well-tested enough that it can be integrated into core Git.
> 
> Can this rationale go in the commit messages?

I thought I had done exactly that in 1/3...

> Actually I wouldn't mind if this were all a single patch, with such a
> rationale in the commit message.

Quite honestly, I'd rather not. There is a logical structure to the three
patches (and I left a Duck in 3/3, to see who bothers to actually read
what I wrote).

The redirection, for example, is a very special thing that I would like to
keep in a separate commit, for clarity.

> The patches' concept seems sane.  I haven't looked closely at the
> implementation.

Thanks,
Dscho

Re: [PATCH 1/2] sequencer: factor out rewrite_file()

[Kevin Daudt]

On Tue, Oct 31, 2017 at 10:54:21AM +0100, René Scharfe wrote:
> Reduce code duplication by extracting a function for rewriting an
> existing file.
> 
> Signed-off-by: Rene Scharfe 
> ---
>  sequencer.c | 46 +-
>  1 file changed, 17 insertions(+), 29 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index f2a10cc4f2..17360eb38a 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2665,6 +2665,20 @@ int check_todo_list(void)
>   return res;
>  }
>  
> +static int rewrite_file(const char *path, const char *buf, size_t len)
> +{
> + int rc = 0;
> + int fd = open(path, O_WRONLY);
> + if (fd < 0)
> + return error_errno(_("could not open '%s' for writing"), path);
> + if (write_in_full(fd, buf, len) < 0)
> + rc = error_errno(_("could not write to '%s'"), path);
> + if (!rc && ftruncate(fd, len) < 0)
> + rc = error_errno(_("could not truncate '%s'"), path);
> + close(fd);
> + return rc;
> +}
> +
>  /* skip picking commits whose parents are unchanged */
>  int skip_unnecessary_picks(void)
>  {
> @@ -2737,29 +2751,11 @@ int skip_unnecessary_picks(void)
>   }
>   close(fd);
>  
> - fd = open(rebase_path_todo(), O_WRONLY, 0666);
> - if (fd < 0) {
> - error_errno(_("could not open '%s' for writing"),
> - rebase_path_todo());
> + if (rewrite_file(rebase_path_todo(), todo_list.buf.buf + offset,
> +  todo_list.buf.len - offset) < 0) {
>   todo_list_release(_list);
>   return -1;
>   }
> - if (write_in_full(fd, todo_list.buf.buf + offset,
> - todo_list.buf.len - offset) < 0) {
> - error_errno(_("could not write to '%s'"),
> - rebase_path_todo());
> - close(fd);
> - todo_list_release(_list);

Is this missing on purpose in the new situation?

> - return -1;
> - }
> - if (ftruncate(fd, todo_list.buf.len - offset) < 0) {
> - error_errno(_("could not truncate '%s'"),
> - rebase_path_todo());
> - todo_list_release(_list);
> - close(fd);
> - return -1;
> - }
> - close(fd);
>  
>   todo_list.current = i;
>   if (is_fixup(peek_command(_list, 0)))
> @@ -2944,15 +2940,7 @@ int rearrange_squash(void)
>   }
>   }
>  
> - fd = open(todo_file, O_WRONLY);
> - if (fd < 0)
> - res = error_errno(_("could not open '%s'"), todo_file);
> - else if (write(fd, buf.buf, buf.len) < 0)
> - res = error_errno(_("could not write to '%s'"), 
> todo_file);
> - else if (ftruncate(fd, buf.len) < 0)
> - res = error_errno(_("could not truncate '%s'"),
> -todo_file);
> - close(fd);
> + res = rewrite_file(todo_file, buf.buf, buf.len);
>   strbuf_release();
>   }
>  
> -- 
> 2.15.0

Except for that question, it looks good to me (as a beginner), it makes
the code better readable.

Re: [PATCH v2] read_index_from(): Skip verification of the cache entry order to speed index loading

[Jeff King]

On Tue, Oct 31, 2017 at 09:01:45AM -0400, Ben Peart wrote:

> > > But what we probably _do_ need is to make sure that "git fsck" would
> > > detect such an out-of-order index. So that developers and users alike
> > > can diagnose suspected problems.
> > 
> > Agree -- that seems like a better home for this logic.
> 
> That is how version 1 of this patch worked but the feedback to that patch
> was to remove it "not only during the normal operation but also in fsck."

Sorry for the mixed messages (I think they are mixed between different
people, and not mixed _just_ from me ;) ).

For what it's worth, I like your v1, but can live with either approach.

-Peff

Re: [PATCH 3/3] mingw: document the experimental standard handle redirection

[Johannes Schindelin]

Hi Eric,

On Mon, 30 Oct 2017, Eric Sunshine wrote:

> On Mon, Oct 30, 2017 at 1:10 PM, Johannes Schindelin
>  wrote:
> > This feature is still highly experimental and has not even been
> > contributed to the Git mailing list yet: the feature still needs to be
> > battle-tested more.
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> > +`GIT_REDIRECT_STDIN`::
> > +`GIT_REDIRECT_STDOUT`::
> > +`GIT_REDIRECT_STDERR`::
> > +   (EXPERIMENTAL) Windows-only: allow redirecting the standard
> > +   input/output/error handles. This is particularly useful in
> > +   multi-threaded applications where the canonical way to pass
> > +   standard handles via `CreateProcess()` is not an option because
> > +   it would require the handles to be marked inheritable (and
> > +   consequently *every* spawned process would inherit them, possibly
> > +   blocking regular Git operations). The primary intended use case
> > +   is to use named pipes for communication.
> > ++
> > +Two special values are supported: `off` will simply close the
> > +corresponding standard handle, and if `GIT_REDIRECT_STDERR` is
> > +`2>&1`, standard error will be redirected to the same handle as
> > +standard output.
> 
> Consistent with the Unixy special-case for '2>&1', I wonder if the
> 'off' case would be more intuitively stated as '>/dev/null' or just
> '/dev/null'...

I feel this is the wrong way round. `>/dev/null` may sound very intuitive
to you, but this feature is Windows only. Guess three times how intuitive
it sounds to Windows developers to write `>/dev/null` if you want to
suppress output...

:0)

Ciao,
Dscho

Re: [PATCH 0/2] Re* Consequences of CRLF in index?

[Jeff King]

On Tue, Oct 31, 2017 at 09:41:25AM -0700, Stefan Beller wrote:

> On Mon, Oct 30, 2017 at 7:44 PM, Junio C Hamano  wrote:
> > Stefan Beller  writes:
> >
> >> (I note this as you regard your patches as a lunch time hack
> >> in the cooking email; I am serious about these patches though.)
> >
> > We do not want to touch borrowed code unnecessarily.  Are these
> > lines and bits hampering further progress we are actively trying to
> > make right now?
> 
> No they are not, you are correct.
> 
> I differ in opinion on 'borrowed code'. The latest release of xdiff
> (v0.23) was in Nov 13, 2008 according to 
> http://freecode.com/projects/xdiff-lib
> or on March 23, 2006 according to https://directory.fsf.org/wiki/LibXDiff
> and given that we incorporated so many changes already to xdiff,
> I would argue it is sufficiently different from the original, we'll probably
> never import another upstream version (if there will be a release at all).
> 
> So the code was rather taken and now we are the bag holders in
> maintaining it, so we can make it pretty even only for the sake of
> pleasing ourselves (or rather: not confusing ourselves with too
> many unused flags).

Yes, I'd agree. For what it's worth both sides of this argument played
out in my head when I saw your patches, and I ended up at the same "we
are the bag holders" place. And there's certainly precedent for touching
that code and cleaning it up to make it easier to work with (just look
at at "git log -p xdiff").

I don't know that I would support a full-scale rewrite (for the same
reason that I wouldn't on the rest of the code base -- avoiding churn).
But deleting unused bits seems like an easy win for readability.

-Peff

Re: [PATCH 0/2] Re* Consequences of CRLF in index?

[Stefan Beller]

On Mon, Oct 30, 2017 at 7:44 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> (I note this as you regard your patches as a lunch time hack
>> in the cooking email; I am serious about these patches though.)
>
> We do not want to touch borrowed code unnecessarily.  Are these
> lines and bits hampering further progress we are actively trying to
> make right now?

No they are not, you are correct.

I differ in opinion on 'borrowed code'. The latest release of xdiff
(v0.23) was in Nov 13, 2008 according to http://freecode.com/projects/xdiff-lib
or on March 23, 2006 according to https://directory.fsf.org/wiki/LibXDiff
and given that we incorporated so many changes already to xdiff,
I would argue it is sufficiently different from the original, we'll probably
never import another upstream version (if there will be a release at all).

So the code was rather taken and now we are the bag holders in
maintaining it, so we can make it pretty even only for the sake of
pleasing ourselves (or rather: not confusing ourselves with too
many unused flags).

Thanks,
Stefan

Re: [PATCH 2/2] sequencer: use O_TRUNC to truncate files

[Kevin Daudt]

On Tue, Oct 31, 2017 at 10:58:16AM +0100, René Scharfe wrote:
> Cut off any previous content of the file to be rewritten by passing the
> flag O_TRUNC to open(2) instead of calling ftruncate(2) at the end.
> That's easier and shorter.
> 
> Signed-off-by: Rene Scharfe 
> ---
>  sequencer.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 17360eb38a..f93b60f615 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2668,13 +2668,11 @@ int check_todo_list(void)
>  static int rewrite_file(const char *path, const char *buf, size_t len)
>  {
>   int rc = 0;
> - int fd = open(path, O_WRONLY);
> + int fd = open(path, O_WRONLY | O_TRUNC);
>   if (fd < 0)
>   return error_errno(_("could not open '%s' for writing"), path);
>   if (write_in_full(fd, buf, len) < 0)
>   rc = error_errno(_("could not write to '%s'"), path);
> - if (!rc && ftruncate(fd, len) < 0)
> - rc = error_errno(_("could not truncate '%s'"), path);
>   close(fd);
>   return rc;
>  }
> -- 
> 2.15.0

Makes sense

How to resolve mixture of modified and deleted conflicts easily in git?

[Robert Dailey]

When doing a rebase, sometimes I will get `DU` and `UU` conflicts
(locally deleted and locally modified, respectively). Furthermore, in
some of these cases, I want to take "ours" for all conflicts,
including ones where the local file is deleted. Ideally, it's just one
command:

$ git checkout --ours .

However, this fails for the locally deleted conflicts:

error: path 'foo.xml' does not have our version

Even more annoyingly, the fact that these failures occur prevents the
`UU` conflicts from being resolved. The whole operation fails
atomically. I am not aware of a straightforward and uniform way to
resolve conflicts with "ours" during a rebase when locally deleted
files exist in the list of conflicts. What is the most elegant
solution in this situation?

I'm running Git for Windows v2.13.1.

Re:

[Amos Kalonzo]

Attn:

I am wondering why You haven't respond to my email for some days now.
reference to my client's contract balance payment of (11.7M,USD)
Kindly get back to me for more details.

Best Regards

Amos Kalonzo

Bug/Wish: bash completion for git pull --rebase doesn't include --autostash

[Albert Astals Cid]

git pull --rebase --autostash

is a valid command but the --autostash autocompletion is not suggested after 
typing

git pul --reb

[PATCH 3/3] sha1_file: use hex_to_bytes()

[René Scharfe]

The path of a loose object contains its hash value encoded into two
substrings of 2 and 38 hexadecimal digits separated by a slash.  The
first part is handed to for_each_file_in_obj_subdir() in decoded form as
subdir_nr.  The current code builds a full hexadecimal representation of
the hash in a temporary buffer, then uses get_oid_hex() to decode it.

Avoid the intermediate step by taking subdir_nr as-is and using
hex_to_bytes() directly on the second substring.  That's shorter and
easier.

Signed-off-by: Rene Scharfe 
---
 sha1_file.c | 24 +++-
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 10c3a0083d..a3c32d91d1 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1884,6 +1884,7 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,
DIR *dir;
struct dirent *de;
int r = 0;
+   struct object_id oid;
 
if (subdir_nr > 0xff)
BUG("invalid loose object subdirectory: %x", subdir_nr);
@@ -1901,6 +1902,8 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,
return r;
}
 
+   oid.hash[0] = subdir_nr;
+
while ((de = readdir(dir))) {
if (is_dot_or_dotdot(de->d_name))
continue;
@@ -1908,20 +1911,15 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,
strbuf_setlen(path, baselen);
strbuf_addf(path, "/%s", de->d_name);
 
-   if (strlen(de->d_name) == GIT_SHA1_HEXSZ - 2)  {
-   char hex[GIT_MAX_HEXSZ+1];
-   struct object_id oid;
-
-   xsnprintf(hex, sizeof(hex), "%02x%s",
- subdir_nr, de->d_name);
-   if (!get_oid_hex(hex, )) {
-   if (obj_cb) {
-   r = obj_cb(, path->buf, data);
-   if (r)
-   break;
-   }
-   continue;
+   if (strlen(de->d_name) == GIT_SHA1_HEXSZ - 2 &&
+   !hex_to_bytes(oid.hash + 1, de->d_name,
+ GIT_SHA1_RAWSZ - 1)) {
+   if (obj_cb) {
+   r = obj_cb(, path->buf, data);
+   if (r)
+   break;
}
+   continue;
}
 
if (cruft_cb) {
-- 
2.15.0

[PATCH 2/3] http-push: use hex_to_bytes()

[René Scharfe]

The path of a loose object contains its hash value encoded into two
substrings of hexadecimal digits, separated by a slash.  The current
code copies the pieces into a temporary buffer to get rid of the slash
and then uses get_oid_hex() to decode the hash value.

Avoid the copy by using hex_to_bytes() directly on the substrings.
That's shorter and easier.

While at it correct the length of the second substring in a comment.

Signed-off-by: Rene Scharfe 
---
 http-push.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/http-push.c b/http-push.c
index 493ee7d719..14435ab65d 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1007,20 +1007,18 @@ static void remote_ls(const char *path, int flags,
  void (*userFunc)(struct remote_ls_ctx *ls),
  void *userData);
 
-/* extract hex from sharded "xx/x{40}" filename */
+/* extract hex from sharded "xx/x{38}" filename */
 static int get_oid_hex_from_objpath(const char *path, struct object_id *oid)
 {
-   char hex[GIT_MAX_HEXSZ];
-
if (strlen(path) != GIT_SHA1_HEXSZ + 1)
return -1;
 
-   memcpy(hex, path, 2);
+   if (hex_to_bytes(oid->hash, path, 1))
+   return -1;
path += 2;
path++; /* skip '/' */
-   memcpy(hex + 2, path, GIT_SHA1_HEXSZ - 2);
 
-   return get_oid_hex(hex, oid);
+   return hex_to_bytes(oid->hash + 1, path, GIT_SHA1_RAWSZ - 1);
 }
 
 static void process_ls_object(struct remote_ls_ctx *ls)
-- 
2.15.0

[PATCH 1/3] notes: move hex_to_bytes() to hex.c and export it

[René Scharfe]

Make the function for converting pairs of hexadecimal digits to binary
available to other call sites.

Signed-off-by: Rene Scharfe 
---
 cache.h |  7 +++
 hex.c   | 12 
 notes.c | 17 -
 3 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/cache.h b/cache.h
index 6440e2bf21..f06bfbaf32 100644
--- a/cache.h
+++ b/cache.h
@@ -1317,6 +1317,13 @@ extern int set_disambiguate_hint_config(const char *var, 
const char *value);
 extern int get_sha1_hex(const char *hex, unsigned char *sha1);
 extern int get_oid_hex(const char *hex, struct object_id *sha1);
 
+/*
+ * Read `len` pairs of hexadecimal digits from `hex` and write the
+ * values to `binary` as `len` bytes. Return 0 on success, or -1 if
+ * the input does not consist of hex digits).
+ */
+extern int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);
+
 /*
  * Convert a binary sha1 to its hex equivalent. The `_r` variant is reentrant,
  * and writes the NUL-terminated output to the buffer `out`, which must be at
diff --git a/hex.c b/hex.c
index 28b44118cb..8df2d63728 100644
--- a/hex.c
+++ b/hex.c
@@ -35,6 +35,18 @@ const signed char hexval_table[256] = {
 -1, -1, -1, -1, -1, -1, -1, -1,/* f8-ff */
 };
 
+int hex_to_bytes(unsigned char *binary, const char *hex, size_t len)
+{
+   for (; len; len--, hex += 2) {
+   unsigned int val = (hexval(hex[0]) << 4) | hexval(hex[1]);
+
+   if (val & ~0xff)
+   return -1;
+   *binary++ = val;
+   }
+   return 0;
+}
+
 int get_sha1_hex(const char *hex, unsigned char *sha1)
 {
int i;
diff --git a/notes.c b/notes.c
index 5c62862574..04f8c8613c 100644
--- a/notes.c
+++ b/notes.c
@@ -334,23 +334,6 @@ static void note_tree_free(struct int_node *tree)
}
 }
 
-/*
- * Read `len` pairs of hexadecimal digits from `hex` and write the
- * values to `binary` as `len` bytes. Return 0 on success, or -1 if
- * the input does not consist of hex digits).
- */
-static int hex_to_bytes(unsigned char *binary, const char *hex, size_t len)
-{
-   for (; len; len--, hex += 2) {
-   unsigned int val = (hexval(hex[0]) << 4) | hexval(hex[1]);
-
-   if (val & ~0xff)
-   return -1;
-   *binary++ = val;
-   }
-   return 0;
-}
-
 static int non_note_cmp(const struct non_note *a, const struct non_note *b)
 {
return strcmp(a->path, b->path);
-- 
2.15.0

[PATCH] t5580: add Cygwin support

[Adam Dinwoodie]

t5580 tests that specifying Windows UNC paths works with Git.  Cygwin
supports UNC paths, albeit only using forward slashes, not backslashes,
so run the compatible tests on Cygwin as well as MinGW.

The only complication is Cygwin's `pwd`, which returns a *nix-style
path, and that's not suitable for calculating the UNC path to the
current directory.  Instead use Cygwin's `cygpath` utility to get the
Windows-style path.

Signed-off-by: Adam Dinwoodie 
---
 t/t5580-clone-push-unc.sh | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
index b322c2f72..47a9a7cda 100755
--- a/t/t5580-clone-push-unc.sh
+++ b/t/t5580-clone-push-unc.sh
@@ -3,12 +3,16 @@
 test_description='various Windows-only path tests'
 . ./test-lib.sh
 
-if ! test_have_prereq MINGW; then
+if test_have_prereq CYGWIN; then
+   alias winpwd='cygpath -aw .'
+elif test_have_prereq MINGW; then
+   alias winpwd=pwd
+else
skip_all='skipping Windows-only path tests'
test_done
 fi
 
-UNCPATH="$(pwd)"
+UNCPATH="$(winpwd)"
 case "$UNCPATH" in
 [A-Z]:*)
# Use administrative share e.g. \\localhost\C$\git-sdk-64\usr\src\git
@@ -45,8 +49,8 @@ test_expect_success push '
test "$rev" = "$(git rev-parse --verify refs/heads/to-push)"
 '
 
-test_expect_success 'remote nick cannot contain backslashes' '
-   BACKSLASHED="$(pwd | tr / )" &&
+test_expect_success MINGW 'remote nick cannot contain backslashes' '
+   BACKSLASHED="$(winpwd | tr / )" &&
git ls-remote "$BACKSLASHED" >out 2>err &&
test_i18ngrep ! "unable to access" err
 '
-- 
2.14.3

RE: What's cooking in git.git (Oct 2017, #07; Mon, 30)

[Jeff Hostetler]

From: Junio C Hamano [mailto:gits...@pobox.com] 
Subject: Re: What's cooking in git.git (Oct 2017, #07; Mon, 30)

> Jeff Hostetler  writes:
> 
>> I've been assuming that the jt/partial-clone-lazy-fetch is a 
>> placeholder for our next combined patch series.
>
> Yes, that, together with the expectation that I will hear from both you and 
> JTan 
> once the result of combined effort becomes ready to replace this placeholder, 
> matches my assumption.
> 
> Is that happening now?

Yes, I'm merging our them now and hope to have a version to
send to Jonathan and/or the list sometime this week.

Jeff

Re: [PATCH v2] read_index_from(): Skip verification of the cache entry order to speed index loading

[Ben Peart]

On 10/30/2017 8:33 PM, Alex Vandiver wrote:

On Mon, 30 Oct 2017, Jeff King wrote:

On Mon, Oct 30, 2017 at 08:48:48AM -0400, Ben Peart wrote:


Any updates or thoughts on this one?  While the patch has become quite
trivial, it does results in a savings of 5%-15% in index load time.


I like the general direction of avoiding the check during each read.


Same -- the savings here are well worth it, IMHO.


I thought the compromise of having this test only run when DEBUG is defined
should limit it to developer builds (hopefully everyone developing on git is
running DEBUG builds :)).  Since the test is trying to detect buggy code
when writing the index, I thought that was the right time to test/catch any
issues.


I certainly don't build with DEBUG. It traditionally hasn't done
anything useful. But I'm also not convinced that this is a likely way to
find bugs in the first place, so I'm OK missing out on it.


I also don't compile with DEBUG -- there's no documentation that
mentions it, and I don't think I'd considered going poking for what
was `#ifdef`d.  I think it'd be reasonable to provide some
configure-time setting that adds `CFLAGS="-ggdb3 -O0 -DDEBUG"` or
similar, but that seems possibly moot for this particular change (see
below).


But what we probably _do_ need is to make sure that "git fsck" would
detect such an out-of-order index. So that developers and users alike
can diagnose suspected problems.


Agree -- that seems like a better home for this logic.


That is how version 1 of this patch worked but the feedback to that 
patch was to remove it "not only during the normal operation but also in 
fsck."





I am working on other, more substantial savings for index load times
(stay tuned) but this seemed like a small simple way to help speed
things up.


I'm interested to hear more about what direction you're looking in here.
  - Alex



I'm working on parallelizing the index load process across multiple 
threads/cpu cores.  Specifically the loop that calls create_from_disk() 
and set_index_entry().  The serial nature of the index formats makes 
that difficult but I believe I've come up with a way to make it work 
across all existing index formats.

Re: [PATCH v2] read_index_from(): Skip verification of the cache entry order to speed index loading

[Ben Peart]

On 10/30/2017 9:49 PM, Junio C Hamano wrote:

Ben Peart  writes:


Any updates or thoughts on this one?  While the patch has become quite
trivial, it does results in a savings of 5%-15% in index load time.

I thought the compromise of having this test only run when DEBUG is
defined should limit it to developer builds (hopefully everyone
developing on git is running DEBUG builds :)).  Since the test is
trying to detect buggy code when writing the index, I thought that was
the right time to test/catch any issues.


This check is more about catching a possible breakage (and a
malicious repository) early before we go too far into the operation.
I do not think this check is about debugging the implementation of
Git.  How would it be useful to turn it on in DEBUG build?

While I do think pursuing any runtime improvements better than a
couple of percents is worth it, I do not think the approach taken by
this iteration makes much sense; the previous one that at least
allowed fsck to catch breakage may have been already too leaky to
catch real issues (i.e. when you are asked to visit and look at an
unknown repository, you wouldn't start your session with "git fsck"
to protect yourself), and this round makes it much worse.

Besides, I see no -DDEBUG from "grep -e '-D[A-Z]*DEBUG' Makefile".
If this check were about allowing us easier time debugging the
binary (which I do not think it is), this probably should be
'#ifndef NDEBUG' instead.



I've tried 3 different ways to remove the overhead of this call from 
regular git operations.


The first was version 1 of the patch which had fsck catch breakage but 
removed it from other commands that read the index.  Since that version 
was not accepted, I took the feedback "I think I like the direction of 
getting rid of the order in post_read_index_from(), not only during the 
normal but also in fsck" to come up with a version 2.


I was hesitant to remove the code completely as I did believe it had 
some value in detecting invalid indexes so went looking for a macro I 
could use to have it 1) not happen during regular user commands but 2) 
still happen for developers.


The NDEBUG macro is guaranteed by the C89 standard 
(http://port70.net/~nsz/c/c89/c89-draft.html#4.1.2 ) to guard the code 
that is only necessary when assertions are in effect so seemed like a 
good choice.  When I used it however, I discovered that the git Makefile 
does not define NDEBUG so using this macro did not have any effect thus 
making the patch useless as the code continues to run in all cases.


On a side note, there are 434 instances of assert which up until this 
experience I believed were being removed in released builds.  As far as 
I can tell, that is not the case.  If removing them is the 
desired/expected behavior, we need to fix our Makefile as it only 
currently defines NDEBUG if USE_NED_ALLOCATOR is defined.


I then searched the code and found 47 instances where the macro DEBUG 
was used.  I (incorrectly) assumed that meant it must be used by other 
git developers.  I personally have a build alias that adds "-j12 
CFLAGS=-DDEBUG" to my make command but apparently I'm in the minority. :)


This assumption led me to the patch version 2 (guarding the code with 
#ifdef DEBUG) as it does meet the request to remove it during normal but 
also fsck and does so with regular/release builds as they are currently 
defined.


It seems that the current round of feedback is more in favor of leaving 
the test in fsck but removing it for other commands.  If that is the 
desired behavior, please use version 1 of the patch.


I'm also happy to flip this to "#ifndef NDEBUG" but that only makes 
sense if the released builds actually set NDEBUG which (I believe) will 
require a patch to Makefile.

[PATCH 2/2] sequencer: use O_TRUNC to truncate files

[René Scharfe]

Cut off any previous content of the file to be rewritten by passing the
flag O_TRUNC to open(2) instead of calling ftruncate(2) at the end.
That's easier and shorter.

Signed-off-by: Rene Scharfe 
---
 sequencer.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 17360eb38a..f93b60f615 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2668,13 +2668,11 @@ int check_todo_list(void)
 static int rewrite_file(const char *path, const char *buf, size_t len)
 {
int rc = 0;
-   int fd = open(path, O_WRONLY);
+   int fd = open(path, O_WRONLY | O_TRUNC);
if (fd < 0)
return error_errno(_("could not open '%s' for writing"), path);
if (write_in_full(fd, buf, len) < 0)
rc = error_errno(_("could not write to '%s'"), path);
-   if (!rc && ftruncate(fd, len) < 0)
-   rc = error_errno(_("could not truncate '%s'"), path);
close(fd);
return rc;
 }
-- 
2.15.0

[PATCH 1/2] sequencer: factor out rewrite_file()

[René Scharfe]

Reduce code duplication by extracting a function for rewriting an
existing file.

Signed-off-by: Rene Scharfe 
---
 sequencer.c | 46 +-
 1 file changed, 17 insertions(+), 29 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index f2a10cc4f2..17360eb38a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2665,6 +2665,20 @@ int check_todo_list(void)
return res;
 }
 
+static int rewrite_file(const char *path, const char *buf, size_t len)
+{
+   int rc = 0;
+   int fd = open(path, O_WRONLY);
+   if (fd < 0)
+   return error_errno(_("could not open '%s' for writing"), path);
+   if (write_in_full(fd, buf, len) < 0)
+   rc = error_errno(_("could not write to '%s'"), path);
+   if (!rc && ftruncate(fd, len) < 0)
+   rc = error_errno(_("could not truncate '%s'"), path);
+   close(fd);
+   return rc;
+}
+
 /* skip picking commits whose parents are unchanged */
 int skip_unnecessary_picks(void)
 {
@@ -2737,29 +2751,11 @@ int skip_unnecessary_picks(void)
}
close(fd);
 
-   fd = open(rebase_path_todo(), O_WRONLY, 0666);
-   if (fd < 0) {
-   error_errno(_("could not open '%s' for writing"),
-   rebase_path_todo());
+   if (rewrite_file(rebase_path_todo(), todo_list.buf.buf + offset,
+todo_list.buf.len - offset) < 0) {
todo_list_release(_list);
return -1;
}
-   if (write_in_full(fd, todo_list.buf.buf + offset,
-   todo_list.buf.len - offset) < 0) {
-   error_errno(_("could not write to '%s'"),
-   rebase_path_todo());
-   close(fd);
-   todo_list_release(_list);
-   return -1;
-   }
-   if (ftruncate(fd, todo_list.buf.len - offset) < 0) {
-   error_errno(_("could not truncate '%s'"),
-   rebase_path_todo());
-   todo_list_release(_list);
-   close(fd);
-   return -1;
-   }
-   close(fd);
 
todo_list.current = i;
if (is_fixup(peek_command(_list, 0)))
@@ -2944,15 +2940,7 @@ int rearrange_squash(void)
}
}
 
-   fd = open(todo_file, O_WRONLY);
-   if (fd < 0)
-   res = error_errno(_("could not open '%s'"), todo_file);
-   else if (write(fd, buf.buf, buf.len) < 0)
-   res = error_errno(_("could not write to '%s'"), 
todo_file);
-   else if (ftruncate(fd, buf.len) < 0)
-   res = error_errno(_("could not truncate '%s'"),
-  todo_file);
-   close(fd);
+   res = rewrite_file(todo_file, buf.buf, buf.len);
strbuf_release();
}
 
-- 
2.15.0

My Greetings

[Mabel Raymond]

My Greetings
MY Name Is Mrs  Raymond.Mabel
I am contacting you  to help me And  My Daughter to  relocate  to your
country My  husband was a serving director of the Gold exporting board
until his death
He Was  killed My the Terrorist Attack Here In the City Of Ouagadougou
The Capital City Of Burkina Faso  He  made a deposit of
($6,500,000.00) here in Ouagadougou Burkina Faso in one of the
Security Finance Company  ,I Discovered The Certificate Of Deposit
About One Week Ago. Which  is  now with me now! i don’t want My Late
husband Relatives To knows About This Fund.  That is the Reason I
contacted You To Help Me  And My Only Daughter To Claim This Fund  Out
>From The Security Company.
Reply to  I will Give you More Details .
Thanks and best regards
 Mrs Raymond.Mabel

[PATCH v5] merge-recursive: check GIT_MERGE_VERBOSITY only once

[Andrey Okoshkin]

Get rid of the duplicated getenv('GIT_MERGE_VERBOSITY') calls with the same
constant string argument. This makes code more readable and prevents typo in
the further development.

Signed-off-by: Andrey Okoshkin 
Reviewed-by: Stefan Beller 
---
Commit message is reworked according to the feedback.
 merge-recursive.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 1494ffdb8..60084e3a0 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2163,6 +2163,7 @@ static void merge_recursive_config(struct merge_options 
*o)
 
 void init_merge_options(struct merge_options *o)
 {
+   const char *merge_verbosity;
memset(o, 0, sizeof(struct merge_options));
o->verbosity = 2;
o->buffer_output = 1;
@@ -2171,9 +2172,9 @@ void init_merge_options(struct merge_options *o)
o->renormalize = 0;
o->detect_rename = 1;
merge_recursive_config(o);
-   if (getenv("GIT_MERGE_VERBOSITY"))
-   o->verbosity =
-   strtol(getenv("GIT_MERGE_VERBOSITY"), NULL, 10);
+   merge_verbosity = getenv("GIT_MERGE_VERBOSITY");
+   if (merge_verbosity)
+   o->verbosity = strtol(merge_verbosity, NULL, 10);
if (o->verbosity >= 5)
o->buffer_output = 0;
strbuf_init(>obuf, 0);
-- 
2.14.3

Re: [PATCH 3/7] builtin/describe.c: rename `oid` to avoid variable shadowing

[Jacob Keller]

On October 30, 2017 5:33:47 PM PDT, Stefan Beller  wrote:
>The function `describe` has already a variable named `oid` declared at
>the beginning of the function for an object id.  Do now shadow that

Nit, s/now/not/

>variable with a pointer to an object id.
>
>Signed-off-by: Stefan Beller 
>---
> builtin/describe.c | 8 
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/builtin/describe.c b/builtin/describe.c
>index 29075dbd0f..fd61f463cf 100644
>--- a/builtin/describe.c
>+++ b/builtin/describe.c
>@@ -381,9 +381,9 @@ static void describe(const char *arg, int last_one)
>   }
> 
>   if (!match_cnt) {
>-  struct object_id *oid = >object.oid;
>+  struct object_id *cmit_oid = >object.oid;
>   if (always) {
>-  printf("%s", find_unique_abbrev(oid->hash, abbrev));
>+  printf("%s", find_unique_abbrev(cmit_oid->hash, 
>abbrev));
>   if (suffix)
>   printf("%s", suffix);
>   printf("\n");
>@@ -392,11 +392,11 @@ static void describe(const char *arg, int
>last_one)
>   if (unannotated_cnt)
>   die(_("No annotated tags can describe '%s'.\n"
>   "However, there were unannotated tags: try 
> --tags."),
>-  oid_to_hex(oid));
>+  oid_to_hex(cmit_oid));
>   else
>   die(_("No tags can describe '%s'.\n"
>   "Try --always, or create some tags."),
>-  oid_to_hex(oid));
>+  oid_to_hex(cmit_oid));
>   }
> 
>   QSORT(all_matches, match_cnt, compare_pt);

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH] rebase: exec leaks GIT_DIR to environment

[Jacob Keller]

On October 30, 2017 5:46:36 AM PDT, Johannes Schindelin 
 wrote:
>Hi Phillip,
>
>On Mon, 30 Oct 2017, Phillip Wood wrote:
>
>> On 30/10/17 06:26, Jacob Keller wrote:
>> > On Sun, Oct 29, 2017 at 8:36 PM, Junio C Hamano 
>wrote:
>> >> Jacob Keller  writes:
>> >>
>> >>> I am pretty confident we can fix it
>> >>
>> >> I am sure we can eventually, but 3 hours is not enough soak time.
>> >>
>> >> I am inclined to leave the fix for 2.15.1/2.16.0 instead of
>delaying
>> >> the release by 10 more days.
>> > 
>> > That's fair. I'm not even sure it was introduced since the last
>> > release (I tried 2.12, but not 2.13 or 2.14 manually). Thus, it
>likely
>> > wasn't noticed for at least a release, meaning it's less important
>(to
>> > me at least) that we provide a fix immediately, since it went
>> > unnoticed this long, likely that means few people will be impacted.
>> 
>> It is in 2.14.3, I haven't bisected but I suspect it was introduced
>by
>> 311af5266b sequencer (rebase -i): implement the 'exec' command
>> 
>> Running
>> git rebase -x'perl -e '\''$,=$\="\n"; print  grep { /^GIT_/ } sort
>keys
>> %ENV'\' @
>> Shows that the rebase--helper version also sets GIT_PREFIX as well as
>> GIT_DIR, I suspect the difference is coming from differences in the
>> setup for builtin commands vs external commands. The shell version
>and
>> the rebase--helper version set GIT_CHERRY_PICK_HELP, GIT_EDITOR,
>> GIT_INTERNAL_GETTEXT_SH_SCHEME, GIT_REFLOG_ACTION
>
>Indeed, when you look at git_dir_init in git-sh-setup, you will see
>that
>Unix shell scripts explicitly get their GIT_DIR turned into an absolute
>path.
>
>So my suggested patch is wrong, and it should be more along the lines
>of
>
>   struct strbuf buf = STRBUF_INIT;
>   const char *child_env[] = { NULL, NULL };
>
>   strbuf_addf(, "GIT_DIR=%s", absolute_path(get_git_dir()));
>   child_env[0] = buf.buf;
>
>   ...
>
>   strbuf_release();
>
>Jake, can I still take you up on taking it from here?
>
>Ciao,
>Dscho

Yes, I will clean this up and submit something tomorrow once I get to a 
computer. I didn't end up having time to work on Git today.

Thanks,
Jake

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH v4] merge-recursive: check GIT_MERGE_VERBOSITY only once

[Junio C Hamano]

Andrey Okoshkin  writes:

> I think, the main benefits are:
> * Code is more readable, no duplicated calls with the same constant string
> argument.
> * Code is potentially safer, the second getenv() call may return another
> pointer value which could be NULL (and yes, this is an arguable point as it
> can be done only artificially).
>
>> For the sake of fairness, I would say that the resulting code may be
>> easier to follow and has one less instance of a constant string that
>> the compiler cannot statically check if we made a typo.  That's the
>> only benefit in this patch as far as I can see.
>> 
>> The original makes a call to see if the result is NULL, and then
>> makes the same call, expecting that we get the same result (not just
>> that it is not NULL, but it is the same verbosity request the end
>> user made via the environment as the one we checked earlier), and I
>> can understand that it feels a bit redundant and ugly.
>
> Yes, you absolutely right.

I am absolutely right when I say your "code is potentially safer" is
total BS.  The result from first getenv() call may be pointing at an
invalid piece of memory by the time it is used, if you are in a
situation in which not having the second getenv() matters
(i.e. somebody else is also mucking with getenv() at the same time).

So please update the log message so that the patch is not sold on
that basis.

Thanks.

Re: [PATCH] diff: --indent-heuristic is no longer experimental

[Junio C Hamano]

Carlos Martín Nieto  writes:

> This heuristic has been the default since 2.14 so we should not confuse our
> users by saying that it's experimental and off by default.
>
> Signed-off-by: Carlos Martín Nieto 
> ---
>  Documentation/diff-heuristic-options.txt | 5 -
>  Documentation/diff-options.txt   | 7 ++-
>  2 files changed, 6 insertions(+), 6 deletions(-)
>  delete mode 100644 Documentation/diff-heuristic-options.txt

I suspect that this patch is incomplete.  The build procedure barfs
and dies while making git-annotate.html, claiming that it wants to
find diff-heuristic-options.txt that no longer exists.

I'll have to redo today's integration cycle again without this
patch.  Sigh...

Re: [PATCH v4] merge-recursive: check GIT_MERGE_VERBOSITY only once

[Andrey Okoshkin]

31.10.2017 05:26, Junio C Hamano wrote:
> Junio C Hamano  writes:
> 
>> That holds true for the code before or after this patch equally.  In
>> other words, that sounds like a justification for rejecting this
>> patch (i.e. explanation of why this change is not needed).
>>
>> If we are worried about another thread calling these functions after
>> the time we call getenv() and before the time we pass the result to
>> strtol(), then I do not think this patch gives a better protection
>> against such race, so I do not think that is why you are doing this.
>>
>> So... why do we want to do this change?  I am puzzled.

I think, the main benefits are:
* Code is more readable, no duplicated calls with the same constant string
argument.
* Code is potentially safer, the second getenv() call may return another
pointer value which could be NULL (and yes, this is an arguable point as it
can be done only artificially).

> For the sake of fairness, I would say that the resulting code may be
> easier to follow and has one less instance of a constant string that
> the compiler cannot statically check if we made a typo.  That's the
> only benefit in this patch as far as I can see.
> 
> The original makes a call to see if the result is NULL, and then
> makes the same call, expecting that we get the same result (not just
> that it is not NULL, but it is the same verbosity request the end
> user made via the environment as the one we checked earlier), and I
> can understand that it feels a bit redundant and ugly.

Yes, you absolutely right.
I believe this patch makes code more beautiful :-)

>>> Signed-off-by: Andrey Okoshkin 
>>> Reviewed-by: Stefan Beller 
>>> ---
>>> Added 'reviewed-by' field.
>>>  merge-recursive.c | 7 ---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/merge-recursive.c b/merge-recursive.c
>>> index 1494ffdb8..60084e3a0 100644
>>> --- a/merge-recursive.c
>>> +++ b/merge-recursive.c
>>> @@ -2163,6 +2163,7 @@ static void merge_recursive_config(struct 
>>> merge_options *o)
>>>  
>>>  void init_merge_options(struct merge_options *o)
>>>  {
>>> +   const char *merge_verbosity;
>>> memset(o, 0, sizeof(struct merge_options));
>>> o->verbosity = 2;
>>> o->buffer_output = 1;
>>> @@ -2171,9 +2172,9 @@ void init_merge_options(struct merge_options *o)
>>> o->renormalize = 0;
>>> o->detect_rename = 1;
>>> merge_recursive_config(o);
>>> -   if (getenv("GIT_MERGE_VERBOSITY"))
>>> -   o->verbosity =
>>> -   strtol(getenv("GIT_MERGE_VERBOSITY"), NULL, 10);
>>> +   merge_verbosity = getenv("GIT_MERGE_VERBOSITY");
>>> +   if (merge_verbosity)
>>> +   o->verbosity = strtol(merge_verbosity, NULL, 10);
>>> if (o->verbosity >= 5)
>>> o->buffer_output = 0;
>>> strbuf_init(>obuf, 0);
> 
> 
> 

-- 
Best regards,
Andrey Okoshkin

Re: [PATCH 4/7] builtin/describe.c: print debug statements earlier

[Junio C Hamano]

Stefan Beller  writes:

> For debuggers aid we'd want to print debug statements early, so
> introduce a new line in the debug output that describes the whole
> function, and the change the next debug output to describe why we need
> to search. Conveniently drop the arg from the second line; which will
> be useful in a follow up commit, that refactors the describe function.
>
> Signed-off-by: Stefan Beller 
> ---

Adding an eary "entry" log may indeed be a good idea.  This is not a
new issue, but I do not think it is a good trade-off to burden i18n
teams to translate debug-only messages, though.

>  builtin/describe.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/describe.c b/builtin/describe.c
> index fd61f463cf..3136efde31 100644
> --- a/builtin/describe.c
> +++ b/builtin/describe.c
> @@ -293,6 +293,9 @@ static void describe(const char *arg, int last_one)
>   unsigned long seen_commits = 0;
>   unsigned int unannotated_cnt = 0;
>  
> + if (debug)
> + fprintf(stderr, _("describe %s\n"), arg);
> +
>   if (get_oid(arg, ))
>   die(_("Not a valid object name %s"), arg);
>   cmit = lookup_commit_reference();
> @@ -316,7 +319,7 @@ static void describe(const char *arg, int last_one)
>   if (!max_candidates)
>   die(_("no tag exactly matches '%s'"), 
> oid_to_hex(>object.oid));
>   if (debug)
> - fprintf(stderr, _("searching to describe %s\n"), arg);
> + fprintf(stderr, _("No exact match on refs or tags, searching to 
> describe\n"));
>  
>   if (!have_util) {
>   struct hashmap_iter iter;

Re: [PATCH 2/7] revision.h: introduce blob/tree walking in order of the commits

[Junio C Hamano]

Stefan Beller  writes:

> diff --git a/list-objects.c b/list-objects.c
> index bf46f80dff..5e114c9a8a 100644
> --- a/list-objects.c
> +++ b/list-objects.c
> @@ -237,6 +237,8 @@ void traverse_commit_list(struct rev_info *revs,
>   if (commit->tree)
>   add_pending_tree(revs, commit->tree);
>   show_commit(commit, data);
> + if (revs->tree_blobs_in_commit_order)
> + traverse_trees_and_blobs(revs, _path, show_object, 
> data);
>   }
>   traverse_trees_and_blobs(revs, _path, show_object, data);

This one is interesting.  While we walk the ancestry chain, we may
add the tree for each commit that we discover to the "pending.  We
used to sweep the pending array at the end after we run out of the
commits, but now we do the sweeping as we process each commit.

Would this make the last call to traverse_trees_and_blobs() always a
no-op?  I am not suggesting to add a new conditional in front of it;
I am just asking for curiosity's sake.

> diff --git a/t/t6100-rev-list-in-order.sh b/t/t6100-rev-list-in-order.sh
> new file mode 100755
> index 00..67ebe815d2
> --- /dev/null
> +++ b/t/t6100-rev-list-in-order.sh
> @@ -0,0 +1,44 @@
> +#!/bin/sh
> +
> +test_description='miscellaneous rev-list tests'
> +
> +. ./test-lib.sh
> +
> +
> +test_expect_success 'setup' '
> + for x in one two three four
> + do
> + echo $x >$x &&
> + git add $x &&
> + git commit -m "add file $x"
> + done &&
> + for x in four three
> + do
> + git rm $x
> + git commit -m "remove $x"
> + done &&

There is break in &&-chain in the second loop, but in any case, for
loops and &&-chains do not mix very well unless done carefully.
Imagine what happens if "git commit" in the first loop failed when
x is two; it won't stop and keep going to create three and four and
nobody would noice any error.

> + git rev-list --in-commit-order --objects HEAD >actual.raw &&
> + cut -c 1-40 > actual < actual.raw &&
> +
> + >expect &&
> + git rev-parse HEAD^{commit}   >>expect &&
> + git rev-parse HEAD^{tree} >>expect &&
> +...
> + git rev-parse HEAD~5^{tree}   >>expect &&

Ooooh, ugly ;-).  I wish we had --stdin interface or something.
Short of that, I'd probably write this more like...

git rev-parse >expect \
A B C D \
E F

Even though we do not have --stdin for rev-parse, you can still do:

git cat-file --batch-check='%(objectname)' >expect <<-\EOF &&
HEAD^{commit}
HEAD^{tree}
HEAD:one
HEAD:two
...
EOF

> + test_cmp expect actual
> +'
> +
> +test_done

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

[Junio C Hamano]

Stefan Beller  writes:

> diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
> index c924c945ba..3d618b2445 100644
> --- a/Documentation/git-describe.txt
> +++ b/Documentation/git-describe.txt
> @@ -3,7 +3,7 @@ git-describe(1)
>  
>  NAME
>  
> -git-describe - Describe a commit using the most recent tag reachable from it
> +git-describe - Describe a commit or blob using the most recent tag reachable 
> from it

If I am not mistaken, this series is about describing a blob as a
tuple of a recent commit-ish and a path in the tree in it.  Blob
never reaches anything, so "desribing blob using X reachable from
it" is a mere nonsense.

The original is not great in that it ignores the "--contains" mode
and referring to "tagged commit" merely as "tag" for brevity, but
at least it made some sense.

> @@ -24,6 +24,16 @@ By default (without --all or --tags) `git describe` only 
> shows
>  annotated tags.  For more information about creating annotated tags
>  see the -a and -s options to linkgit:git-tag[1].
>  
> +If the given `` refers to a blob, it will be described

Perhaps this step should update the SYNOPSIS so that the command
takes not just commit-ish but a blob too.  Given the difficulty in
coming up with the single-liner description of what it does we saw
above, I suspect that splitting SYNOPSIS out into two very distinct
operating mode might make it easier to read.

SYNOPSIS

[verse]
'git describe' [--all] [--tags] [--contains] [--abbrev=] 
[...]
   +'git describe' [...] ...

Then this additional paragraph can say "When describin a ",
without using a (technically nonsense) phrase "if 
refers to a blob", which is never true.

Re: [PATCH 1/7] list-objects.c: factor out traverse_trees_and_blobs

[Junio C Hamano]

Stefan Beller  writes:

> With traverse_trees_and_blobs factored out of the main traverse function,
> the next patch can introduce an in-order revision walking with ease.
>
> The variable holding the base path is only used in the newly factored out
> function `traverse_trees_and_blobs`, however we keep its scope to
> `traverse_commit_list` to keep the number of invocations for memory
> allocations and release to one per commit traversal.

In this step, the caller is calling traverse_trees_and_blobs() only
once per traversal anyway, so the paragraph does not quite justify
this somewhat strange calling convention, unless the reader knows
more calls will be made to the same function by the caller in later
steps, potentially doubling the chance the buffer can be reused with
this arrangement.

Even after this function gains more callers, wouldn't the same
invariant that "path" recorded for trees and blobs are relative to
the root of tree recorded in the object in revs->pending.objects[]?
IOW, should base_path->len be always 0 upon entry to this function?

We may want to have an explicit _reset() or assert(->len == 0) at
the beginning of the function to ensure that.

> Rename the variable to `base_path` for clarity.

Hmph.  Inside traverse_commit_list(), which never looks at the
contents of the buffer, it could just be called with a name that
does not have any meaning (e.g. "scratch area for the callee to
use"), and such a change would make it clear why it is defined one
level above its use and the caller never looks at it.

In the callee (i.e. inside traverse_trees_and_blobs()), however, I
do not see a reason why base_path is any clearer than base.

Other than that, this looks like a straight-forward code movement
and looks good.