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

[Junio C Hamano]

Jonathan Nieder  writes:

>> 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?
>
> Actually I wouldn't mind if this were all a single patch, with such a
> rationale in the commit message.
>
> The patches' concept seems sane.  I haven't looked closely at the
> implementation.

+1.

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

Good eyes.  Nobody raised noises since this happened at 2.14 until
now, so this could wait until the next cycle, though ;-)

>  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
>
> diff --git a/Documentation/diff-heuristic-options.txt 
> b/Documentation/diff-heuristic-options.txt
> deleted file mode 100644
> index d4f3d95505..00
> --- a/Documentation/diff-heuristic-options.txt
> +++ /dev/null
> @@ -1,5 +0,0 @@
> ---indent-heuristic::
> ---no-indent-heuristic::
> - These are to help debugging and tuning experimental heuristics
> - (which are off by default) that shift diff hunk boundaries to
> - make patches easier to read.
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index a88c76741e..dd0dba5b1d 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -63,7 +63,12 @@ ifndef::git-format-patch[]
>   Synonym for `-p --raw`.
>  endif::git-format-patch[]
>  
> -include::diff-heuristic-options.txt[]
> +--indent-heuristic::
> + Enable the heuristic that shift diff hunk boundaries to make patches
> + easier to read. This is the default.
> +
> +--no-indent-heuristic::
> + Disable the indent heuristic.
>  
>  --minimal::
>   Spend extra time to make sure the smallest possible

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

[Junio C Hamano]

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.

Re: future of the mediawiki extension?

[Junio C Hamano]

Antoine Beaupré  writes:

> On 2017-10-30 11:29:55, Matthieu Moy wrote:
>>> It should also be mentioned that this contrib isn't very active: I'm not
>>> part of the GitHub organization, yet I'm probably the one that's been
>>> the most active with patches in the last year (and I wasn't very active
>>> at all).
>>
>> FYI, I'm no longer using Mediawiki as much as I did, and I don't really
>> use Git-Mediawiki anymore.
>>
>> The main blocking point to revive Git-Mediawiki is to find a new
>> maintainer (https://github.com/Git-Mediawiki/Git-Mediawiki/issues/33). I
>> believe I just found one ;-).
>
> Eh. I assume you mean me here. As I hinted at in another thread, I am
> not sure I can commit to leading the project - just scratching an
> itch. But I may be able to review pull requests and make some releases
> from time to time... I probably won't work on code or features I don't
> need unless someone funds my work or something. ;)
>
> We'll see where the community takes us, I guess... Always better to have
> more than one maintainer, anyways, just for the bus factor... Worst
> case, I'll delegate to a worthy successor. :)

Heh, when you are talking about going from effectively 0 to 1 (or a
halftime), you are way too early to worry about the bus factor ;-)

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

[Junio C Hamano]

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.

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

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

[Junio C Hamano]

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

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

Re: [PATCH v2 3/4] diff: add flag to indicate textconv was set via cmdline

[Junio C Hamano]

Brandon Williams  writes:

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

The original is equivalent to 

if (! (DIFF_OPT_TOUCHED() && DIFF_OPT_TST()))
return stream_blob_to_fd();

which means that we must have used DIFF_OPT_SET() or DIFF_OPT_CLR()
to touch the ALLOW_TEXTCONV bit, and ALLOW_TEXTCONV bit is currently
set, in order for the flow to skip this "just stream it out".

And the way it implemented it was:

#define DIFF_OPT_TOUCHED(opts, flag)((opts)->touched_flags & 
DIFF_OPT_##flag)
#define DIFF_OPT_SET(opts, flag)(((opts)->flags |= 
DIFF_OPT_##flag),((opts)->touched_flags |= DIFF_OPT_##flag))
#define DIFF_OPT_CLR(opts, flag)(((opts)->flags &= 
~DIFF_OPT_##flag),((opts)->touched_flags |= DIFF_OPT_##flag))

Notice that touched_flags is SET in both OPT_SET() and OPT_CLR(),
because the point of _TOUCHED() was "did the user made an explicit
request to affect the value of the bit from the command line?".

> diff --git a/diff.c b/diff.c
> index 3ad9c9b31..8b700b1bd 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4762,11 +4762,13 @@ 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_CLR(options, TEXTCONV_SET_VIA_CMDLINE);
> + } else if (!strcmp(arg, "--ignore-submodules")) {

If we were aiming for faithful conversion, the above must be
DIFF_OPT_SET(options, TEXTCONV_SET_VIA_CMDLINE), not CLR.

HOWEVER, I think it is fine to define TEXTCONV_SET_VIA_CMDLINE bit
differently from what DIFF_OPT_TOUCHED(ALLOW_TEXTCONV) meant in the
old code (i.e. "did the user made an explicit request to affect the
value?").  That is, we can define the new one as "did the user
explicitly SET the bit from the command line?", the conditional in
show_blob_object() is prepared to take either interpretation.  "User
explicitly set the bit to true, and the bit is true" and "User
explicitly set the bit to something, and the bit is true" are pretty
much the same thing.

And that leads me to suggest dropping the last change here, to touch
VIA_CMDLINE in response to "--no-textconv".

Other than that, looks good to me.

Thanks.

Re: [PATCH v2 2/4] diff: convert flags to be stored in bitfields

[Junio C Hamano]

Brandon Williams  writes:

> + if (flags)
> + rev.diffopt.flags = diff_flags_or(, flags);

If we are avoiding from passing a struct (even if it is a small one)
by value, then returning a struct as value defeats the point of the
exercise, I would think.  If that will be the calling cconvention,
making diff_flags_or(, ) to update  by or'ing bits in  would
be more natural.

Having said that, as I said in a separate message, as long as we
have this _or() thing, no sane person would add anything but
bitfields to the structure which will guarantee that it will stay to
be small set of flags and nothing else---so I personally am fine
with pass by value (which in turn makes it OK to return as a value,
too).

Other than that, this step looked reasonable to me.

Thanks.

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

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

Meaning of two commit-ish hash in git diff

[Yubin Ruan]

Hi,

Can anyone tell me what does the "f8886b4..a1c96df" mean in a git diff output,
as below?

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.

And BTW, are there any special meanings for that .. ?

Thanks,
Yubin

Re: [PATCH v5 0/4] status: add option to show ignored files differently

[Junio C Hamano]

jameson.mille...@gmail.com writes:

> Only difference from previous version is to update the commit author
> email to match corporate email address.

Will replace; thanks.

Re: [PATCH 3/3] diff: convert flags to be stored in bitfields

[Junio C Hamano]

Junio C Hamano  writes:

> Junio C Hamano  writes:
>
>> I still haven't brought myself to like the structure being passed by
>> value and the singleton diff_flags_cleared thing, but I suspect that
>> we may get used to them once we start using these.  I dunno.
>
> Just bikeshedding, but I just had to prepare an evil merge to add a
> new use of diff_flags_cleared to a codepath that evolved in a topic
> still in flight, and realized that I really hate the name.  Perhaps
> I wouldn't have hated it so much if it were named diff_flags_none or
> diff_flags_empty, I guess.

As to the "passing by value" thing, as long as we have the _or()
helper function, no sensible person will add anything but a bitfield
into the structure, so I am fine with it.

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

[Junio C Hamano]

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?

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

[Junio C Hamano]

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?

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

[Junio C Hamano]

Johannes Schindelin  writes:

> Hi Junio,
>
> 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.

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

[Junio C Hamano]

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.

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.


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

Re: future of the mediawiki extension?

[Antoine Beaupré]

On 2017-10-31 10:37:29, Junio C Hamano wrote:
>> There's also a hybrid solution used by git-multimail: have a copy of the
>> code in git.git, but do the development separately. I'm not sure it'd be
>> a good idea for Git-Mediawiki, but I'm mentionning it for completeness.
>
> I think the plan was to make code drop from time to time at major
> release points of git-multimail, but I do not think we've seen many
> updates recently.

I'd be okay with a hybrid as well. It would require minimal work on
Git's side at this stage: things can just stay as is until there's a new
"release" of the mediawiki extension and at that point you can decide if
you merge it all in or if you drop it in favor of the contrib.

I think it's also fine to punt it completely out to the community.

Either way, I may have time to do some of that work in the coming month,
so let me know what you prefer, I guess you two have the last word
here. The community, on Mediawiki's side, seem to mostly favor GitHub.

A.

-- 
Never attribute to malice that which can be adequately explained by
stupidity, but don't rule out malice.
 - Albert Einstein

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

Andrey Okoshkin  writes:

> Get 'GIT_MERGE_VERBOSITY' environment variable only once in
> init_merge_options() and store the pointer to its value for the further check.

OK, that is "what this thing does" description.

> No intervening calls to getenv(), putenv(), setenv() or unsetenv() are done
> between the initial getenv() call and the consequential result pass to 
> strtol()
> as these environment related functions could modify the string pointer 
> returned
> by the initial getenv() call.

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.


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

Re: future of the mediawiki extension?

[Junio C Hamano]

Matthieu Moy  writes:

> So, my conclusion is that a simpler submission mechanism (GitHub's
> pull-requests) and a less picky code review would help Git-Mediawiki.
>
> From previous discussions, I think Junio will agree with that: he's
> reluctant to keeping too much stuff in contrib/ and usally prefers
> external projects.
>
> Note that being a separate project doesn't mean there can't be any
> interaction with this list. Requests for reviews for separate projects
> are usually welcome even though they don't happen often here.

I would say that Git and its ecosystem has become mature enough that
any add-on project that aims to make life more pleasant for those
who use Git and $X together for any value of $X can now stand on its
own, without being under Git umbrella like back in the days when the
number of people who know and/or use Git were small.  The world is
no longer constrained by small number of people with Git expertise,
and it has become practical to discuss their project among those who
are familiar with (and motivated to learn) *both* Git and $X without
necessarily involving Git 'core' people.

Participants of this list will continue to strive to keep this list
the place for people to come for Git expertise.  But this list may
no longer be the best place to find those who are experts on *both*
Git and $X.  And that is why I think an external project standing on
its own would be more preferrable these days.

> There's also a hybrid solution used by git-multimail: have a copy of the
> code in git.git, but do the development separately. I'm not sure it'd be
> a good idea for Git-Mediawiki, but I'm mentionning it for completeness.

I think the plan was to make code drop from time to time at major
release points of git-multimail, but I do not think we've seen many
updates recently.

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

[brian m. carlson]

On Mon, Oct 30, 2017 at 01:28:05PM +0900, 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.
> 
> > @@ -58,8 +65,9 @@ differs substantially from the prior version, are all 
> > good things
> >  to have.
> >  
> >  Make sure that you have tests for the bug you are fixing.  See
> > -t/README for guidance.
> > +'t/README' for guidance.
> 
> I am guessing that, from the way you updated 'next' to `next'
> etc. in the previous hunk, you are typesetting in  anything a
> reader may type literally without substitution.
> 
> Should this be `t/README`, as it is a part of something a reader may
> type literally (as in "less t/README")?

So this syntax provides italicized paths, but I agree that the  is
better here.  I'll make those changes throughout, and fix up the
instances of that you mentioned.

> > -(4) Sending your patches.
> > +[[send-patches]]
> > +=== Sending your patches.
> > +:1: footnote:[The current maintainer: gits...@pobox.com]
> > +:2: footnote:[The mailing list: git@vger.kernel.org]
> 
> Having to see these footnotes upfront is somewhat distracting for
> those of us who prefer to use this file as a text document.  I see
> these become part of the footnotes section at the very end of the
> document (as opposed to the end of this section), so even with the
> rendered output it does not look ideal.
> 
> I am not sure how much these two points matter, though.

AsciiDoc requires that the attributes appear before their references.  I
can move the attributes just before the paragraph they refer to, or I
can inline the footnotes.  I could also just turn them into links if
that works better.

This is actually one thing that I think Markdown does better.

> > @@ -191,7 +212,7 @@ not ready to be applied but it is for discussion, 
> > [PATCH v2],
> > ..
> >  Send your patch with "To:" set to the mailing list, with "cc:" listing
> >  people who are involved in the area you are touching (the output from
> > -"git blame $path" and "git shortlog --no-merges $path" would help to
> > ++git blame _$path_+ and +git shortlog {litdd}no-merges _$path_+ would help 
> > to
> >  identify them), to solicit comments and reviews.
> 
> The +fixed width with _italics_ mixed in+ mark-up is something not
> exactly new, but it is rarely used in our documentation set, so I
> had to double check by actually seeing how it got rendered, and it
> looked alright.

I thought it provided some hint to the reader that this wasn't meant to
be typed literally.  It's a preference of mine and I think it aids in
readability, but it can be changed if we want.

> >  After the list reached a consensus that it is a good idea to apply the
> > -patch, re-send it with "To:" set to the maintainer [*1*] and "cc:" the
> > -list [*2*] for inclusion.
> > +patch, re-send it with "To:" set to the maintainer{1} and "cc:" the
> > +list{2} for inclusion.
> >  
> >  Do not forget to add trailers such as "Acked-by:", "Reviewed-by:" and
> >  "Tested-by:" lines as necessary to credit people who helped your
> >  patch.
> 
> Should these "Foo-by:" all be `Foo-by:`?

I'll make these changes as well.

> > -An ideal patch flow
> > +[[patch-flow]]
> > +== An ideal patch flow
> >  
> >  Here is an ideal patch flow for this project the current maintainer
> >  suggests to the contributors:
> >  
> > - (0) You come up with an itch.  You code it up.
> > +. You come up with an itch.  You code it up.
> >  
> > - (1) Send it to the list and cc people who may need to know about
> > - the change.
> > +. Send it to the list and cc people who may need to know about
> > +  the change.
> > ++
> > +The people who may need to know are the ones whose code you
> > +are butchering.  These people happen to be the ones who are
> > +most likely to be knowledgeable enough to help you, but
> > +they have no obligation to help you (i.e. you ask for help,
> > +don't demand).  +git log -p {litdd} _$area_you_are_modifying_+ would
> > +help you find out who they are.
> >  
> > - The people who may need to know are the ones whose code you
> > - are butchering.  These people happen to be the ones who are
> > - most likely to be knowledgeable enough to help you, but
> > - they have no obligation to help you (i.e. you ask for help,
> > - don't demand).  "git log -p -- $area_you_are_modifying" would
> > - help you find out who they are.
> > +. You get comments and suggestions for improvements.  You may
> > +  even get them in a "on top of your change" patch form.
> >  
> > - (2) You get comments and suggestions for improvements.  You may
> > - even get them in a "on top of your change" patch form.
> > +. Polish, refine, and re-send to the list and the people who
> > +  spend their time to improve your 

[PATCH 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

[PATCH 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.

Helped-by: Johannes Schindelin 
Signed-off-by: Stefan Beller 
---
 list-objects.c   |  2 ++
 revision.c   |  2 ++
 revision.h   |  3 ++-
 t/t6100-rev-list-in-order.sh | 44 
 4 files changed, 50 insertions(+), 1 deletion(-)
 create mode 100755 t/t6100-rev-list-in-order.sh

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);
 
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..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 &&
+   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^{tree}:one >>expect &&
+   git rev-parse HEAD^{tree}:two >>expect &&
+   git rev-parse HEAD~1^{commit} >>expect &&
+   git rev-parse HEAD~1^{tree}   >>expect &&
+   git rev-parse HEAD~1^{tree}:three >>expect &&
+   git rev-parse HEAD~2^{commit} >>expect &&
+   git rev-parse HEAD~2^{tree}   >>expect &&
+   git rev-parse HEAD~2^{tree}:four  >>expect &&
+   git rev-parse HEAD~3^{commit} >>expect &&
+   # skip HEAD~3^{tree}
+   git rev-parse HEAD~4^{commit} >>expect &&
+   # skip HEAD~4^{tree}
+   git rev-parse HEAD~5^{commit} >>expect &&
+   git rev-parse HEAD~5^{tree}   >>expect &&
+
+   test_cmp expect actual
+'
+
+test_done
-- 
2.15.0.rc2.443.gfcc3b81c0a

[PATCH 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 now 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

[PATCH 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.

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.  Rename the variable
to `base_path` for clarity.

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

diff --git a/list-objects.c b/list-objects.c
index b3931fa434..bf46f80dff 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -183,25 +183,13 @@ 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);
-   }
for (i = 0; i < revs->pending.nr; i++) {
struct object_array_entry *pending = revs->pending.objects + i;
struct object *obj = pending->item;
@@ -218,17 +206,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 base_path;
+   strbuf_init(_path, 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, _path, show_object, data);
+
+   strbuf_release(_path);
 }
-- 
2.15.0.rc2.443.gfcc3b81c0a

[PATCH 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 | 12 +++-
 builtin/describe.c | 65 ++
 t/t6120-describe.sh| 15 ++
 3 files changed, 86 insertions(+), 6 deletions(-)

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
 
 
 SYNOPSIS
@@ -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
+as `:`, such that the blob can be found
+at `` in the `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);
+   strbuf_addf(pcd->dst, ":%s", path);
+   }
+}
+
+static void describe_blob(struct object_id oid, struct strbuf *dst)
+{
+   struct rev_info revs;
+   struct argv_array args = 

[PATCH 0/7] git-describe

[Stefan Beller]

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

[1] 
https://stackoverflow.com/questions/10622179/how-to-find-identify-large-files-commits-in-git-history

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 |  12 +++-
 builtin/describe.c | 125 -
 list-objects.c |  50 ++---
 revision.c |   2 +
 revision.h |   3 +-
 t/t6100-rev-list-in-order.sh   |  44 +++
 t/t6120-describe.sh|  17 +-
 7 files changed, 203 insertions(+), 50 deletions(-)
 create mode 100755 t/t6100-rev-list-in-order.sh

-- 
2.15.0.rc2.443.gfcc3b81c0a

[PATCH 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

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

[Alex Vandiver]

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.

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

[PATCH 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

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

[Brandon Williams]

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?

> @@ -77,6 +77,61 @@ struct object_id {
>   unsigned char hash[GIT_MAX_RAWSZ];
>  };
>  
> +/*
> + * Note that these constants are suitable for indexing the hash_algos array 
> and
> + * comparing against each other, but are otherwise arbitrary, so they should 
> not
> + * be exposed to the user or serialized to disk.  To know whether a
> + * git_hash_algo struct points to some usable hash function, test the 
> format_id
> + * field for being non-zero.  Use the name field for user-visible situations 
> and
> + * the format_id field for fixed-length fields on disk.
> + */
> +/* An unknown hash function. */
> +#define GIT_HASH_UNKNOWN 0
> +/* SHA-1 */
> +#define GIT_HASH_SHA1 1
> +/* Number of algorithms supported (including unknown). */
> +#define GIT_HASH_NALGOS (GIT_HASH_SHA1 + 1)
> +
> +typedef void (*git_hash_init_fn)(void *ctx);
> +typedef void (*git_hash_update_fn)(void *ctx, const void *in, size_t len);
> +typedef void (*git_hash_final_fn)(unsigned char *hash, void *ctx);
> +
> +struct git_hash_algo {
> + /*
> +  * The name of the algorithm, as appears in the config file and in
> +  * messages.
> +  */
> + const char *name;
> +
> + /* A four-byte version identifier, used in pack indices. */
> + uint32_t format_id;
> +
> + /* The size of a hash context (e.g. git_SHA_CTX). */
> + size_t ctxsz;
> +
> + /* The length of the hash in binary. */
> + size_t rawsz;
> +
> + /* The length of the hash in hex characters. */
> + size_t hexsz;
> +
> + /* The hash initialization function. */
> + git_hash_init_fn init_fn;
> +
> + /* The hash update function. */
> + git_hash_update_fn update_fn;
> +
> + /* The hash finalization function. */
> + git_hash_final_fn final_fn;
> +
> + /* The OID of the empty tree. */
> + const struct object_id *empty_tree;
> +
> + /* The OID of the empty blob. */
> + const struct object_id *empty_blob;
> +};
> +extern const struct git_hash_algo hash_algos[GIT_HASH_NALGOS];
> +
>  #if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)
>  #define DTYPE(de)((de)->d_type)
>  #else
> diff --git a/sha1_file.c b/sha1_file.c
> index 10c3a0083d..77b320126a 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -39,6 +39,49 @@ const struct object_id empty_blob_oid = {
>   EMPTY_BLOB_SHA1_BIN_LITERAL
>  };
>  
> +static inline void git_hash_sha1_init(void *ctx)
> +{
> + git_SHA1_Init((git_SHA_CTX *)ctx);
> +}
> +
> +static inline void git_hash_sha1_update(void *ctx, const void *data, size_t 
> len)
> +{
> + git_SHA1_Update((git_SHA_CTX *)ctx, data, len);
> +}
> +
> +static inline void git_hash_sha1_final(unsigned char *hash, void *ctx)
> +{
> + git_SHA1_Final(hash, (git_SHA_CTX *)ctx);
> +}
> +
> +const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {
> + {
> + NULL,
> + 0x,
> + 0,
> + 0,
> + 0,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + },
> + {
> + "sha-1",
> + /* "sha1", big-endian */
> 

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

[brian m. carlson]

On Mon, Oct 30, 2017 at 01:35:19PM +0100, Johannes Schindelin wrote:
> 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)).

The thing I really like about AsciiDoc is that it works well for a
variety of output formats.  Markdown is designed for HTML, and only
HTML.  It may have converters to other formats, but then you can't use
any extension mechanisms in HTML.  Markdown also lacks features that
AsciiDoc has, like cross-references, named anchors, and the ability to
write the linkgit syntax.

AsciiDoc, via DocBook and the XSLT stylesheets, supports conversion to
PDF, DVI, PS, and ePub, among others.  The things I'm seeing for
Markdown to PDF involve either Pandoc or a browser engine such as
phantomjs.

Also, AsciiDoc has the benefit that it has only two implementations.
Markdown has so many variants that it's hard to write things like tables
in a portable way, so we're going to have at least as many problems
(between the website and the codebase) as we do now.
-- 
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

[PATCH 0/2] New send-email option --quote-email

[Payre Nathan]

Those patches implements a new --quote-email= option.

Typical use case: the user receives a bug report by email and replies with a 
patch.
Before this patch, to make a proper reply, the user had to perform
several steps manually using "git send-email":

* Add --in-reply-to= to the command-line for proper
  threading.

* Include the original recipients of the message using --to and --cc.

* Copy and prefix the original message with '> ' in the "below triple
  dash" part of the patch.

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.

Based-on-patch-by: Tom Russello 
Signed-off: Nathan Payre 
Signed-off: Matthieu Moy 

Tom Russello (2):
  quote-email populates the fields
  send-email: quote-email quotes the message body

 Documentation/git-send-email.txt |   5 ++
 git-send-email.perl  | 146 +--
 t/t9001-send-email.sh| 134 ---
 3 files changed, 240 insertions(+), 45 deletions(-)

-- 
2.14.2

[PATCH 2/2] send-email: quote-email quotes the message body

[Payre Nathan]

From: Tom Russello 

---
 Documentation/git-send-email.txt |  4 +-
 git-send-email.perl  | 80 ++--
 t/t9001-send-email.sh| 19 +-
 3 files changed, 97 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 710b5ff32..329af66af 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -107,7 +107,9 @@ 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.
+   Fill appropriately header fields for the reply to the given email and 
quote
+   the message body in the cover letter if `--compose` is set or otherwise
+   after the triple-dash in the first patch given.
 
 --subject=::
Specify the initial subject of the email thread.
diff --git a/git-send-email.perl b/git-send-email.perl
index 665c47d15..6f6995c9d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -26,6 +26,7 @@ use Text::ParseWords;
 use Term::ANSIColor;
 use File::Temp qw/ tempdir tempfile /;
 use File::Spec::Functions qw(catdir catfile);
+use File::Copy;
 use Error qw(:try);
 use Cwd qw(abs_path cwd);
 use Git;
@@ -57,7 +58,8 @@ 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.
+--quote-email* Populate header fields appropriately and
+ quote the message body.
 --[no-]xmailer * Add "X-Mailer:" header (default).
 --[no-]annotate* Review each patch that will be sent in an 
editor.
 --compose  * Open an editor for introduction.
@@ -654,12 +656,15 @@ if (@files) {
usage();
 }
 
+my $message_quoted;
 if ($quote_email) {
my $error = validate_patch($quote_email);
die "fatal: $quote_email: $error\nwarning: no patches were sent\n"
if $error;
 
my @header = ();
+   my $date;
+   my $recipient;
 
open my $fh, "<", $quote_email or die "can't open file $quote_email";
 
@@ -691,7 +696,8 @@ if ($quote_email) {
}
$initial_subject = $prefix_re . $subject_re;
} elsif (/^From:\s+(.*)$/i) {
-   push @initial_to, $1;
+   $recipient = $1;
+   push @initial_to, $recipient;
} elsif (/^To:\s+(.*)$/i) {
foreach my $addr (parse_address_line($1)) {
if (!($addr eq $initial_sender)) {
@@ -713,9 +719,28 @@ if ($quote_email) {
$initial_reply_to = $1;
} elsif (/^References:\s+(.*)/i) {
$initial_references = $1;
+   } elsif (/^Date: (.*)/i) {
+   $date = $1;
}
}
$initial_references = $initial_references . $initial_reply_to;
+
+   my $tpl_date = $date && "On $date, " || '';
+   $message_quoted = $tpl_date . $recipient . " wrote:\n";
+
+   # Quote the message body
+   while (<$fh>) {
+   # Turn crlf line endings into lf-only
+   s/\r//g;
+   my $space = "";
+   if (/^[^>]/) {
+   $space = " ";
+   }
+   $message_quoted .= ">" . $space . $_;
+   }
+   if (!$compose) {
+   $annotate = 1;
+   }
 }
 
 sub get_patch_subject {
@@ -743,6 +768,9 @@ if ($compose) {
my $tpl_sender = $sender || $repoauthor || $repocommitter || '';
my $tpl_subject = $initial_subject || '';
my $tpl_reply_to = $initial_reply_to || '';
+   my $tpl_quote = $message_quoted &&
+   "\nGIT: Please, trim down irrelevant sections in the quoted 
message\n".
+   "GIT: to keep your email concise.\n" . $message_quoted || '';
 
print $c < $repo->repo_path()) :
+   tempfile(".gitsendemail.msg.XX",
+   DIR => "."))[1];
+
+   # Insertion in a temporary file to keep the 

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

[Payre Nathan]

From: Tom Russello 

---
 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.
+
 --subject=::
Specify the initial subject of the email thread.
Only necessary if --compose is also set.  If --compose
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.
 --[no-]xmailer * Add "X-Mailer:" header (default).
 --[no-]annotate* Review each patch that will be sent in an 
editor.
 --compose  * Open an editor for introduction.
@@ -166,7 +167,7 @@ my $re_encoded_word = 
qr/=\?($re_token)\?($re_token)\?($re_encoded_text)\?=/;
 
 # Variables we fill in automatically, or via prompting:
 my (@to,$no_to,@initial_to,@cc,$no_cc,@initial_cc,@bcclist,$no_bcc,@xh,
-   $initial_reply_to,$initial_subject,@files,
+   
$initial_reply_to,$initial_references,$quote_email,$initial_subject,@files,
$author,$sender,$smtp_authpass,$annotate,$use_xmailer,$compose,$time);
 
 my $envelope_sender;
@@ -316,6 +317,7 @@ $rc = GetOptions(
"sender|from=s" => \$sender,
 "in-reply-to=s" => \$initial_reply_to,
"subject=s" => \$initial_subject,
+   "quote-email=s" => \$quote_email,
"to=s" => \@initial_to,
"to-cmd=s" => \$to_cmd,
"no-to" => \$no_to,
@@ -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;
+
+   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) {
+   chomp($header[$#header]);
+   s/^\s+/ /;
+   $header[$#header] .= $_;
+   } else {
+   push(@header, $_);
+   }
+   }
+
+   # Parse the header
+   foreach (@header) {
+   my $initial_sender = $sender || $repoauthor || $repocommitter 
|| '';
+
+   chomp;
+
+   if (/^Subject:\s+(.*)$/i) {
+   my $prefix_re = "";
+   my $subject_re = $1;
+   if ($1 =~ /^[^Re:]/) {
+   $prefix_re = "Re: ";
+   }
+   $initial_subject = $prefix_re . $subject_re;
+   } elsif (/^From:\s+(.*)$/i) {
+   push @initial_to, $1;
+   } elsif (/^To:\s+(.*)$/i) {
+   foreach my $addr (parse_address_line($1)) {
+   if (!($addr eq $initial_sender)) {
+   push @initial_cc, $addr;
+   }
+   }
+   } elsif (/^Cc:\s+(.*)$/i) {
+   foreach my $addr (parse_address_line($1)) {
+   my $qaddr = unquote_rfc2047($addr);
+   my $saddr = sanitize_address($qaddr);
+   if ($saddr eq $initial_sender) {
+   next if ($suppress_cc{'self'});
+   } else {
+   next if ($suppress_cc{'cc'});
+   }
+   push @initial_cc, $addr;
+   }
+   } elsif (/^Message-Id: (.*)/i) {
+   $initial_reply_to = $1;
+   } elsif (/^References:\s+(.*)/i) {
+   $initial_references = $1;
+   }
+   }
+   $initial_references = $initial_references . $initial_reply_to;
+}
+
 sub 

Re: [PATCH 00/13] WIP Partial clone part 1: object filtering

[Jonathan Nieder]

Hi,

Jonathan Tan wrote:

> As for how this patch set (excluding the partialclone patch) interacts
> with my fsck series, they are relatively independent, as far as I can
> tell. I'll rebase my fsck, gc, and lazy object fetch patches (but not
> the fetch and clone parts, which we plan to instead adapt from Jeff
> Hostetler's patches, as far as I know) on top of these and resend
> those out once discussion on this has settled.

Selfishly, I'll make a request here.  The only part of the series that
overlaps is the max-blob-bytes part, right?  Would you mind re-sending
the remainder of the series so it can go through the "next" ->
"master" -> etc process in the usual way?

My selfishness comes in because this would reduce the set of patches I
have to apply locally to just the max-blob-bytes part.  If I
understood correctly, the rest of the series was something everyone
agreed about, so there's no reason not to pursue including it in
"next".

I'd have the same request for this object filtering series, but I
think it's already happening: the patches in this thread so far do not
allow omitting some blobs from the local object store, so they should
be able to go through the "next" -> "master" -> etc process as well
without having to wait for the fsck patches.

Thanks,
Jonathan

[PATCH v2 7/4] 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| 30 +++---
 diff.h|  2 --
 merge-recursive.c |  2 +-
 revision.c|  4 ++--
 submodule.c   |  6 +++---
 7 files changed, 23 insertions(+), 25 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 16d33c319..0e5abb5ce 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,13 +4761,13 @@ 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")) {

[PATCH v2 6/4] 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 v2 5/4] 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 v2 8/4] 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| 172 +-
 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, 229 insertions(+), 229 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)

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

[Jonathan Nieder]

Hi,

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?

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

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

Thanks,
Jonathan

Re: [PATCH v2 3/4] diff: add flag to indicate textconv was set via cmdline

[Brandon Williams]

On 10/30, Brandon Williams wrote:
> On 10/30, Stefan Beller wrote:
> > On Mon, Oct 30, 2017 at 12:46 PM, Brandon Williams  
> > wrote:
> > > 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 requested
> > > via the command line.
> > 
> > Is it worth mentioning 4197361e39 (Merge branch 'mg/more-textconv',
> > 2013-10-23), that introduced the touched_flags?
> > (+cc Michael Gruber FYI)
> > 
> > > Signed-off-by: Brandon Williams 
> > > ---
> > >  builtin/log.c | 2 +-
> > >  diff.c| 8 +---
> > >  diff.h| 1 +
> > >  3 files changed, 7 insertions(+), 4 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..8b700b1bd 100644
> > > --- a/diff.c
> > > +++ b/diff.c
> > > @@ -4762,11 +4762,13 @@ 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);
> > 
> > Also clear TEXTCONV_SET_VIA_CMDLINE here?
> > (`git show --textconv --no-textconv` might act funny?)
> 
> Oops, thanks for catching this, I added it in the wrong place! (as you
> can see below.

wait nevermind, i overreacted.  And this patch does correctly clear
TEXTCONV_SET_VIA_CMDLINE.

> 
> > 
> > > -   else if (!strcmp(arg, "--ignore-submodules")) {
> > > +   DIFF_OPT_CLR(options, TEXTCONV_SET_VIA_CMDLINE);
> > > +   } 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=", )) {
> > > diff --git a/diff.h b/diff.h
> > > index 47e6d43cb..4eaf9b370 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
> > >
> 
> -- 
> Brandon Williams

-- 
Brandon Williams

Re: [PATCH v2 3/4] diff: add flag to indicate textconv was set via cmdline

[Brandon Williams]

On 10/30, Stefan Beller wrote:
> On Mon, Oct 30, 2017 at 12:46 PM, Brandon Williams  wrote:
> > 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 requested
> > via the command line.
> 
> Is it worth mentioning 4197361e39 (Merge branch 'mg/more-textconv',
> 2013-10-23), that introduced the touched_flags?
> (+cc Michael Gruber FYI)
> 
> > Signed-off-by: Brandon Williams 
> > ---
> >  builtin/log.c | 2 +-
> >  diff.c| 8 +---
> >  diff.h| 1 +
> >  3 files changed, 7 insertions(+), 4 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..8b700b1bd 100644
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -4762,11 +4762,13 @@ 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);
> 
> Also clear TEXTCONV_SET_VIA_CMDLINE here?
> (`git show --textconv --no-textconv` might act funny?)

Oops, thanks for catching this, I added it in the wrong place! (as you
can see below.

> 
> > -   else if (!strcmp(arg, "--ignore-submodules")) {
> > +   DIFF_OPT_CLR(options, TEXTCONV_SET_VIA_CMDLINE);
> > +   } 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=", )) {
> > diff --git a/diff.h b/diff.h
> > index 47e6d43cb..4eaf9b370 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
> >

-- 
Brandon Williams

Re: [PATCH v2 3/4] diff: add flag to indicate textconv was set via cmdline

[Stefan Beller]

On Mon, Oct 30, 2017 at 12:46 PM, Brandon Williams  wrote:
> 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 requested
> via the command line.

Is it worth mentioning 4197361e39 (Merge branch 'mg/more-textconv',
2013-10-23), that introduced the touched_flags?
(+cc Michael Gruber FYI)

> Signed-off-by: Brandon Williams 
> ---
>  builtin/log.c | 2 +-
>  diff.c| 8 +---
>  diff.h| 1 +
>  3 files changed, 7 insertions(+), 4 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..8b700b1bd 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4762,11 +4762,13 @@ 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);

Also clear TEXTCONV_SET_VIA_CMDLINE here?
(`git show --textconv --no-textconv` might act funny?)

> -   else if (!strcmp(arg, "--ignore-submodules")) {
> +   DIFF_OPT_CLR(options, TEXTCONV_SET_VIA_CMDLINE);
> +   } 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=", )) {
> diff --git a/diff.h b/diff.h
> index 47e6d43cb..4eaf9b370 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
>

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

[Jeff Hostetler]

On 10/30/2017 1:31 PM, Johannes Schindelin wrote:

Hi Junio,

On Mon, 30 Oct 2017, Junio C Hamano wrote:


* jt/partial-clone-lazy-fetch (2017-10-02) 18 commits
  - fetch-pack: restore save_commit_buffer after use
  - unpack-trees: batch fetching of missing blobs
  - clone: configure blobmaxbytes in created repos
  - clone: support excluding large blobs
  - fetch: support excluding large blobs
  - fetch: refactor calculation of remote list
  - fetch-pack: support excluding large blobs
  - pack-objects: support --blob-max-bytes
  - pack-objects: rename want_.* to ignore_.*
  - gc: do not repack promisor packfiles
  - rev-list: support termination at promisor objects
  - sha1_file: support lazily fetching missing objects
  - introduce fetch-object: fetch one promisor object
  - index-pack: refactor writing of .keep files
  - fsck: support promisor objects as CLI argument
  - fsck: support referenced promisor objects
  - fsck: support refs pointing to promisor objects
  - fsck: introduce partialclone extension

  A journey for "git clone" and "git fetch" to become "lazier" by
  depending more on its remote repository---this is the beginning of
  it.

  Expecting a reroll.
  cf. 


It was my understanding that Jeff's heavy-lifting produced a shorter,
initial patch series with parts of this, that was already reviewed
internally by Jonathan.

Am I mistaken?

Ciao,
Dscho



Right.  I posted a "part 1" of this last week and am currently
rerolling that.  I should also have a followup "part 2" patch
series shortly.

https://public-inbox.org/git/20171024185332.57261-1-...@jeffhostetler.com/

I've been assuming that the jt/partial-clone-lazy-fetch is a
placeholder for our next combined patch series.

Jeff

[PATCH v2 3/4] 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 requested
via the command line.

Signed-off-by: Brandon Williams 
---
 builtin/log.c | 2 +-
 diff.c| 8 +---
 diff.h| 1 +
 3 files changed, 7 insertions(+), 4 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..8b700b1bd 100644
--- a/diff.c
+++ b/diff.c
@@ -4762,11 +4762,13 @@ 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_CLR(options, TEXTCONV_SET_VIA_CMDLINE);
+   } 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=", )) {
diff --git a/diff.h b/diff.h
index 47e6d43cb..4eaf9b370 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 v2 4/4] 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 4eaf9b370..7e567108e 100644
--- a/diff.h
+++ b/diff.h
@@ -111,9 +111,8 @@ static inline struct diff_flags diff_flags_or(const 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)
@@ -142,7 +141,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 v2 2/4] 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   | 97 +---
 sequencer.c  |  5 +--
 6 files changed, 72 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..6c1c05c5b 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)
+   rev.diffopt.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..47e6d43cb 100644
--- a/diff.h
+++ b/diff.h
@@ -60,42 +60,60 @@ 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 

[PATCH v2 0/4] convert diff flags to be stored in a struct

[Brandon Williams]

Changes in v2:
 * removed the diff_flags_cleared singleton 
 * eliminated the 'touched' parallel flags
 * pass structs by reference instead of by value

Now that the 'touched' flags have been removed it may be valuable to go ahead
and remove the macros all together (including making the flags lower case).  If
that's what we want to do, I can go ahead and send those patches out as a
follow on to these.

Brandon Williams (4):
  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

 builtin/add.c|  2 +-
 builtin/commit.c |  7 +++--
 builtin/log.c|  3 +-
 builtin/reset.c  |  2 +-
 diff-lib.c   |  7 +++--
 diff.c   | 10 +++---
 diff.h   | 96 +---
 sequencer.c  |  5 +--
 8 files changed, 77 insertions(+), 55 deletions(-)

-- 
2.15.0.403.gc27cc4dac6-goog

[PATCH v2 1/4] 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

Re: [PATCH 3/3] diff: convert flags to be stored in bitfields

[Brandon Williams]

On 10/30, Junio C Hamano wrote:
> Junio C Hamano  writes:
> 
> > I still haven't brought myself to like the structure being passed by
> > value and the singleton diff_flags_cleared thing, but I suspect that
> > we may get used to them once we start using these.  I dunno.
> 
> Just bikeshedding, but I just had to prepare an evil merge to add a
> new use of diff_flags_cleared to a codepath that evolved in a topic
> still in flight, and realized that I really hate the name.  Perhaps
> I wouldn't have hated it so much if it were named diff_flags_none or
> diff_flags_empty, I guess.

I have a new version of the series I'll send out and i ended up dropping
it entirely.  Didn't even need a clear function because I was able to
drop the touched stuff and it would have only been used inside of
builtin/log.c to clear the touched flags.

-- 
Brandon Williams

Re: [PATCH 2/2] diff.c: get rid of duplicate implementation

[Jeff King]

On Mon, Oct 30, 2017 at 10:59:41AM -0700, Jeff King wrote:

> > There's also https://www.strchr.com/hash_functions, which lists DJB2
> > as Bernstein.  Both functions rank somewhere in the middle of that list.
> 
> FWIW, I did some experiments with Murmur3 and SipHash a while back, but
> I don't think I came up with anything faster than the existing code.
> OTOH, moving to SipHash gives us the ability to randomize the hashes,
> which can resist some DoS attacks (although as I've said before,
> computing arbitrary diffs for untrusted strangers is pretty much a
> DoS-in-a-box).

By the way, one of the things that complicates plugging new functions
into xdiff's hashing is that xdl_hash_record() simultaneously computes
the hash _and_ finds the end-of-line marker.

So the "siphash is only 10% slower" number I showed came with quite a
few contortions to do both. Here it is compared to a more naive
application of the siphash code (i.e., memchr to find end-of-line, and
then feed the resulting bytes to siphash):

Test  originHEAD^   
 jk/xdl-siphash-wip
---
4000.1: log -3000 (baseline)  0.05(0.05+0.00)   0.05(0.05+0.00) +0.0%   
 0.05(0.05+0.00) +0.0%
4000.2: log --raw -3000 (tree-only)   0.31(0.27+0.03)   0.31(0.27+0.03) +0.0%   
 0.31(0.27+0.03) +0.0%
4000.3: log -p -3000 (Myers)  2.06(2.01+0.05)   2.30(2.21+0.09) +11.7%  
 2.96(2.91+0.04) +43.7%
4000.4: log -p -3000 --histogram  2.44(2.38+0.06)   2.67(2.60+0.07) +9.4%   
 3.32(3.26+0.06) +36.1%
4000.5: log -p -3000 --patience   2.57(2.47+0.09)   2.90(2.82+0.08) +12.8%  
 3.48(3.43+0.05) +35.4%

There "origin" is the existing djb hash, "HEAD^" is the complicated
"fast" siphash (which I very well may have screwed up), and the final is
the more naive version, which is quite a bit slower.

-Peff

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

[Eric Sunshine]

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

On the nature of cover letters [WAS Re: [PATCH 0/6] Create Git/Packet.pm]

[Stefan Beller]

On Mon, Oct 30, 2017 at 11:08 AM, Jeff King  wrote:
> On Mon, Oct 23, 2017 at 01:26:41PM +0100, Philip Oakley wrote:
>
>> > Totally offtopic, but is it only me who finds these "section
>> > headers" in cover letters from some people irritating and/or
>> > jarring?
>>
>> Personally I find that, for significant patch series, that clearly breaking
>> out these distinct sections is of advantage. At this stage (the very first
>> patch 0/n) there is no specific conversation, so the subject line is a short
>> 'hello' to the topic, and then the contributor is (it is to be hoped)
>> setting out their proposal in a clear manner.
>>
>> So I do like these headings for larger series, though there is some
>> judgement to be made as to when the subject line alone is sufficient.
>
> I can live with fancily-formatted cover letters. BUT. I would say if
> your cover letter is getting quite long, you might consider whether some
> of its content ought to be going elsewhere (either into commit messages
> themselves, or into a design document or other place inside the repo).

I am of the opinion that in an ideal workflow the cover letter would be
part of the merge commit message. It may even be editorialized or
annotated by the maintainer performing the merge, but not necessarily so.

Currently I rarely pay attention to merges, because they are not exciting
(in a good way). I mostly know the texts that Junio puts into the merge
message[1] from the cooking email, and otherwise there is not much
information added there.

However looking at *what* Junio puts there, is "why is this worthwhile
to merge from the *projects* point of view?", whereas the cover letter
most of the time talks about "Why the *contributor* wants this merged".
Often these are subtly different, so it would be nice to have these
two competing views around.

[1] e.g. cf. da15b78e52 Merge branch 'jk/ui-color-always-to-auto'

>> As a separate follow on, one thing that does annoy me is that in subsequent
>> versions of the various patch series, folk tend to drop all explanation of
>> why the series is of any relevance, leaving just the 'changed since last
>> time' part. This means that new readers who try and pick up / review /
>> contribute to a series later on in its development are not told the purpose.
>> When the list is active it can, accidentally, do a disservice to the
>> potential contributors who may feel that only core contributors are able to
>> contribute.
>
> I actually have the opposite opinion. I find it annoying to have to wade
> through the same unchanged content for each round just to find the
> little snippet of "here's what's changed".

Would you be happier if the "What changed?" goes first[2]?
Though I think whether to just reply to the previous version, put an
explicit link or copy the cover letter verbatim from last time is up
for discussion. I tent to think a link ought to be enough, because
those familiar with the topic would not follow it (so they have no
additional burden compared to direct copy), and new comers to
that topic may be ok with links .

[2] I tried following that style, e.g.
https://public-inbox.org/git/20170630205310.7380-1-sbel...@google.com/


> I don't mind following a link to the previous iteration to read the
> back-story if I wasn't involved (it's a good idea to do that anyway to
> see what previous reviews have already discussed).

Such a back story would make an excellent merge message, too,
as it explains the big picture more accurately, often shows
alternatives considered etc.

Stefan

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

[Jeff King]

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.

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

-Peff

Re: [PATCH 0/6] Create Git/Packet.pm

[Jeff King]

On Mon, Oct 23, 2017 at 01:26:41PM +0100, Philip Oakley wrote:

> > Totally offtopic, but is it only me who finds these "section
> > headers" in cover letters from some people irritating and/or
> > jarring?
> 
> Personally I find that, for significant patch series, that clearly breaking
> out these distinct sections is of advantage. At this stage (the very first
> patch 0/n) there is no specific conversation, so the subject line is a short
> 'hello' to the topic, and then the contributor is (it is to be hoped)
> setting out their proposal in a clear manner.
> 
> So I do like these headings for larger series, though there is some
> judgement to be made as to when the subject line alone is sufficient.

I can live with fancily-formatted cover letters. BUT. I would say if
your cover letter is getting quite long, you might consider whether some
of its content ought to be going elsewhere (either into commit messages
themselves, or into a design document or other place inside the repo).

> As a separate follow on, one thing that does annoy me is that in subsequent
> versions of the various patch series, folk tend to drop all explanation of
> why the series is of any relevance, leaving just the 'changed since last
> time' part. This means that new readers who try and pick up / review /
> contribute to a series later on in its development are not told the purpose.
> When the list is active it can, accidentally, do a disservice to the
> potential contributors who may feel that only core contributors are able to
> contribute.

I actually have the opposite opinion. I find it annoying to have to wade
through the same unchanged content for each round just to find the
little snippet of "here's what's changed".

I don't mind following a link to the previous iteration to read the
back-story if I wasn't involved (it's a good idea to do that anyway to
see what previous reviews have already discussed).

I do often just post my "v2" as a follow-up and assume people can find
the original by following the thread backwards. But I imagine that not
everybody can do so. It's probably a good practice to at least put a
link to the prior version (and also to v1 for the original motivation)
if you're not going to repeat the cover letter in full.

-Peff

Re: [PATCH 2/3] reset: use DIFF_OPT_SET macro to set a diff flag

[Brandon Williams]

On 10/29, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > Instead of explicitly setting the 'DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG'
> > flag, use the 'DIFF_OPT_SET' macro.
> >
> > Signed-off-by: Brandon Williams 
> > ---
> 
> Looks good.  It's not like one of 1/3 and 2/3 could be a good idea
> while the other is not, so it would make a lot more sense to combine
> them into a single preliminary clean-up patch, though.  
> 

I'll squash them together in v2.

> In any case, these two are very good clean-up patches, whose value
> does not diminish even we do not go ahead with 3/3 yet.  
> 
> Nicely spotted; thanks.
> 
> 
> >  builtin/reset.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > 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;

-- 
Brandon Williams

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

[Jeff King]

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

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

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.

-Peff

Re: [PATCH 2/2] diff.c: get rid of duplicate implementation

[Jeff King]

On Thu, Oct 26, 2017 at 07:43:19PM +0200, René Scharfe wrote:

> Am 25.10.2017 um 20:49 schrieb Stefan Beller:
> > The implementations in diff.c to detect moved lines needs to compare
> > strings and hash strings, which is implemented in that file, as well
> > as in the xdiff library.
> > 
> > Remove the rather recent implementation in diff.c and rely on the well
> > exercised code in the xdiff lib.
> > 
> > With this change the hash used for bucketing the strings for the moved
> > line detection changes from FNV32 (that is provided via the hashmaps
> > memhash) to DJB2 (which is used internally in xdiff).  Benchmarks found
> > on the web[1] do not indicate that these hashes are different in
> > performance for readable strings.
> > 
> > [1] 
> > https://softwareengineering.stackexchange.com/questions/49550/which-hashing-algorithm-is-best-for-uniqueness-and-speed
> 
> Awesome comparison!  It calls the variant used in libxdiff DJB2a (which
> uses xor to mix in the new char) instead of DJB2 (which uses addition).
> 
> There's also https://www.strchr.com/hash_functions, which lists DJB2
> as Bernstein.  Both functions rank somewhere in the middle of that list.

FWIW, I did some experiments with Murmur3 and SipHash a while back, but
I don't think I came up with anything faster than the existing code.
OTOH, moving to SipHash gives us the ability to randomize the hashes,
which can resist some DoS attacks (although as I've said before,
computing arbitrary diffs for untrusted strangers is pretty much a
DoS-in-a-box).

Anyway, I rebased them and ran p4000[1], with does show some results:

Test  originjk/xdl-murmur3-wip  
 jk/xdl-siphash-wip
---
4000.1: log -3000 (baseline)  0.05(0.05+0.00)   0.05(0.05+0.00) +0.0%   
 0.05(0.05+0.00) +0.0% 
4000.2: log --raw -3000 (tree-only)   0.30(0.28+0.02)   0.30(0.28+0.01) +0.0%   
 0.30(0.28+0.02) +0.0% 
4000.3: log -p -3000 (Myers)  2.06(1.98+0.08)   1.90(1.85+0.05) -7.8%   
 2.32(2.25+0.07) +12.6%
4000.4: log -p -3000 --histogram  2.50(2.42+0.07)   2.25(2.21+0.04) -10.0%  
 2.70(2.62+0.08) +8.0% 
4000.5: log -p -3000 --patience   2.58(2.52+0.06)   2.47(2.42+0.04) -4.3%   
 2.86(2.77+0.08) +10.9%

So it looks like murmur3 is indeed a little faster. SipHash is slower,
which is too bad (because the randomization would be nice). I _think_
back then I compared to XDL_FAST_HASH, which was a little faster even
than murmur3 (but generated too many collisions, which led to bad
behavior for certain cases).

Anyway, those branches are at https://github.com/peff/git if anybody
wants to look further. They probably need some cleanup. At the very
least, we'd probably need to teach the whitespace-ignoring hash function
to use the same algorithm.

-Peff

[1] Actually, I added "--no-merges" to each command in p4000. It seems
silly as it is, since the point is to compute a bunch of diffs.

Re: [PATCH 3/3] diff: convert flags to be stored in bitfields

[Brandon Williams]

On 10/29, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > We have have reached the limit of the number of flags that can be stored
> 
> s/have have/have/; but bit #9 is unused.  
> 
> "We cannot add many more flags even if we wanted to" would be more
> flexible and does not take this change hostage to whatever topic
> that tries to claim that bit, I would think.

I'll tweak the wording a bit.

> 
> > 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|  2 +-
> >  diff-lib.c   |  6 ++--
> >  diff.c   |  3 +-
> >  diff.h   | 96 
> > +---
> >  sequencer.c  |  5 +--
> >  6 files changed, 70 insertions(+), 49 deletions(-)
> >
> 
> > diff --git a/diff.h b/diff.h
> > index aca150ba2..d58f06106 100644
> > --- a/diff.h
> > +++ b/diff.h
> > @@ -60,42 +60,59 @@ 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_FUNCCONTEXT (1 << 29)
> > -#define DIFF_OPT_PICKAXE_IGNORE_CASE (1 << 30)
> > -#define DIFF_OPT_DEFAULT_FOLLOW_RENAMES (1U << 31)
> > -
> > -#define DIFF_OPT_TST(opts, flag)((opts)->flags & DIFF_OPT_##flag)
> > -#define DIFF_OPT_TOUCHED(opts, flag)((opts)->touched_flags & 
> > DIFF_OPT_##flag)
> > -#define DIFF_OPT_SET(opts, flag)(((opts)->flags |= 
> > DIFF_OPT_##flag),((opts)->touched_flags |= DIFF_OPT_##flag))
> > -#define DIFF_OPT_CLR(opts, flag)(((opts)->flags &= 
> > ~DIFF_OPT_##flag),((opts)->touched_flags |= DIFF_OPT_##flag))
> > +#define DIFF_FLAGS_INIT { 0 }
> > +extern const struct diff_flags diff_flags_cleared;
> 
> This thing is curious.  Seeing the scary diff_flags_or(), I would
> have expected we'd have diff_flags_clear().

This seemed to make things easier in practice because I was passing some
structs by value, let me work with it a bit to see what it looks like
when they are passed by reference instead.

> 
> > +struct diff_flags {
> > +   unsigned RECURSIVE:1;
> > +   unsigned TREE_IN_RECURSIVE:1;
> > +   unsigned BINARY:1;
> > +...
> > +   unsigned FUNCCONTEXT:1;
> > +   unsigned PICKAXE_IGNORE_CASE:1;
> > +   unsigned DEFAULT_FOLLOW_RENAMES:1;
> > +};
> > +
> > +static inline struct diff_flags diff_flags_or(struct diff_flags a,
> > + struct diff_flags b)
> > +{
> > +   char *tmp_a = (char *)
> > +   char *tmp_b = (char *)
> > +   int i;
> > +
> > +   for (i = 0; i < sizeof(struct diff_flags); i++)
> > +   tmp_a[i] |= tmp_b[i];
> > +
> > +   return a;
> > +}
> 
> This is doubly scary, but let's see why we need it by looking at the
> callers.
> 
> > +#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))
> 
> These are trivial and straight-forward.
> 
> > +
> >  #define DIFF_XDL_TST(opts, flag)((opts)->xdl_opts & XDF_##flag)
> >  #define DIFF_XDL_SET(opts, flag)((opts)->xdl_opts |= XDF_##flag)
> >  #define DIFF_XDL_CLR(opts, flag)((opts)->xdl_opts &= ~XDF_##flag)
> > @@ -122,8 +139,8 @@ struct diff_options {
> > const char *a_prefix, *b_prefix;
> > const char *line_prefix;
> > size_t line_prefix_length;
> > -   unsigned flags;
> > -   unsigned touched_flags;
> > +   struct diff_flags flags;
> > +   struct diff_flags touched_flags;
> >  
> > /* diff-filter bits */
> > unsigned int filter;
> > @@ -388,7 +405,8 @@ extern int diff_result_code(struct diff_options *, int);
> >  
> >  extern void diff_no_index(struct rev_info *, int, const char **);
> >  
> > -extern int index_differs_from(const char *def, int diff_flags, int 
> > ita_invisible_in_index);
> > +extern int index_differs_from(const char *def, struct diff_flags flags,
> > + int ita_invisible_in_index);
> 
> OK.  I tend to think twice before passing any struct by value (even
> something that starts its life as a small/single-word struct), but
> let's see how much simpler this allows callers to become.
> 
> > diff --git a/builtin/commit.c b/builtin/commit.c
> > index d75b3805e..de08c2594 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 

Re: [PATCH v16 Part II 6/8] bisect--helper: `get_terms` & `bisect_terms` shell function in C

[Pranit Bauva]

Hey Martin,

On Sat, Oct 28, 2017 at 1:34 AM, Martin Ågren  wrote:
> On 27 October 2017 at 17:06, Pranit Bauva  wrote:
>> +   for (i = 0; i < argc; i++) {
>> +   if (!strcmp(argv[i], "--term-good"))
>> +   printf(_("%s\n"), terms->term_good);
>> +   else if (!strcmp(argv[i], "--term-bad"))
>> +   printf(_("%s\n"), terms->term_bad);
>
> You seem to have lost --term-old and --term-new. I also wonder why these
> would need translating. You break GETTEXT_POISON here, then fix it in
> patch 8/8.
>
> I'm not even sure you need patch 8/8. If I drop these two `_()`, I can
> run `git rebase -ix "make GETTEXT_POISON=Yes test"` on the entire series
> without failure. Patch 8/8 also switches to `test_i18ngrep` for some
> usages of `git branch` and for some checks on `.git/BISECT_START`. I'm
> not sure that's needed.

Maybe. I am not quite familiar with what does GETTEXT_POISON exactly
does, so I will probably investigate further in this.

Regards,
Pranit Bauva

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

[Pranit Bauva]

Hey Martin,

On Fri, Oct 27, 2017 at 11:05 PM, Martin Ågren  wrote:
> On 27 October 2017 at 17:06, Pranit Bauva  wrote:
>> +   /*
>> +* have bad (or new) but not good (or old). We could bisect
>> +* although this is less optimum.
>> +*/
>> +   fprintf(stderr, _("Warning: bisecting only with a %s 
>> commit\n"),
>> +   terms->term_bad);
>
> Maybe this should use `warning()`?

Yeah. That would be better.

>> -   # have bad (or new) but not good (or old).  we could bisect 
>> although
>> -   # this is less optimum.
>> -   eval_gettextln "Warning: bisecting only with a \$TERM_BAD 
>> commit." >&2
>
> I wonder if we can somehow pick up the existing translation? It would
> now be fuzzy, in some sense, but since the string was originally in a
> different file, maybe the po-tools won't be able to discover the
> fuzzyness? We could add a TRANSLATORS-comment, so that the translators
> know that this string matches an old one. There are more strings like
> that in this patch, and maybe in some others as well, I haven't looked.
>
> (Adding Jiang to cc.)

Since I am re-rolling my previous series as well, I can make the
change in all patches.

Regards,
Pranit Bauva

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

[Paolo Ciarrocchi]

On Mon, Oct 30, 2017 at 1:35 PM, Johannes Schindelin
 wrote:
> Hi,
>
> 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 :-)

Ciao,
-- 
Paolo

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

[Stefan Beller]

On Sun, Oct 29, 2017 at 8:12 AM, Carlos Martín Nieto  wrote:
> 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 

Looks good to me,

Thanks,
Stefan

Re: [PATCH] t0000: check whether the shell supports the "local" keyword

[Jeff King]

On Thu, Oct 26, 2017 at 10:18:53AM +0200, Michael Haggerty wrote:

> Add a test balloon to see if we get complaints from anybody who is
> using a shell that doesn't support the "local" keyword. If so, this
> test can be reverted. If not, we might want to consider using "local"
> in shell code throughout the git code base.
> 
> Signed-off-by: Michael Haggerty 
> ---
> This has been discussed on the mailing list [1,2].

Thanks for following up, this looks nice and thorough to me.

-Peff

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

[Pranit Bauva]

Hey Junio,

On Fri, Oct 27, 2017 at 11:49 PM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>> - bisect_write "$state" "$rev"
>> + git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" 
>> "$TERM_BAD" || exit
>
> I can see why two extra "terms" parameters need to be passed to this
> helper at this step; looking at patches around 4/8 and 6/8 where C
> code can directly find out what words are used for GOOD and BAD, we
> should be able to lose these two extra parameters from this helper
> by internally making a call to get_terms() from bisect_write() ;-)

Yes quite true, but then after converting bisect_skip() we can
completely get rid of this line and then it won't be needed in the
ported C code.

PS: I have already ported that function but those patches are local as of now.

Regards,
Pranit Bauva

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

[Pranit Bauva]

Hey Martin,

On Fri, Oct 27, 2017 at 10:58 PM, Martin Ågren  wrote:
> On 27 October 2017 at 17:06, Pranit Bauva  wrote:
>> +static void free_terms(struct bisect_terms *terms)
>> +{
>> +   if (!terms->term_good)
>> +   free((void *) terms->term_good);
>> +   if (!terms->term_bad)
>> +   free((void *) terms->term_bad);
>> +}
>
> These look like no-ops. Remove `!` for correctness, or `if (...)` for
> simplicity, since `free()` can handle NULL.

I probably forgot to do this here. I will make the change.

> You leave the pointers dangling, but you're ok for now since this is the
> last thing that happens in `cmd_bisect__helper()`. Your later patches
> add more users, but they're also ok, since they immediately assign new
> values.
>
> In case you (and others) find it useful, the below is a patch I've been
> sitting on for a while as part of a series to plug various memory-leaks.
> `FREE_AND_NULL_CONST()` would be useful in precisely these situations.

Honestly, I wouldn't be the best person to judge this.

Regards,
Pranit Bauva

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

[Johannes Schindelin]

Hi Junio,

On Mon, 30 Oct 2017, Junio C Hamano wrote:

> * jt/partial-clone-lazy-fetch (2017-10-02) 18 commits
>  - fetch-pack: restore save_commit_buffer after use
>  - unpack-trees: batch fetching of missing blobs
>  - clone: configure blobmaxbytes in created repos
>  - clone: support excluding large blobs
>  - fetch: support excluding large blobs
>  - fetch: refactor calculation of remote list
>  - fetch-pack: support excluding large blobs
>  - pack-objects: support --blob-max-bytes
>  - pack-objects: rename want_.* to ignore_.*
>  - gc: do not repack promisor packfiles
>  - rev-list: support termination at promisor objects
>  - sha1_file: support lazily fetching missing objects
>  - introduce fetch-object: fetch one promisor object
>  - index-pack: refactor writing of .keep files
>  - fsck: support promisor objects as CLI argument
>  - fsck: support referenced promisor objects
>  - fsck: support refs pointing to promisor objects
>  - fsck: introduce partialclone extension
> 
>  A journey for "git clone" and "git fetch" to become "lazier" by
>  depending more on its remote repository---this is the beginning of
>  it.
> 
>  Expecting a reroll.
>  cf. 

It was my understanding that Jeff's heavy-lifting produced a shorter,
initial patch series with parts of this, that was already reviewed
internally by Jonathan.

Am I mistaken?

Ciao,
Dscho

Re: [PATCH v16 Part II 6/8] bisect--helper: `get_terms` & `bisect_terms` shell function in C

[Pranit Bauva]

Hey Stephan,

On Mon, Oct 30, 2017 at 10:04 PM, Stephan Beyer  wrote:
> On 10/27/2017 05:06 PM, Pranit Bauva wrote:
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> index 0f9c3e63821b8..ab0580ce0089a 100644
>> --- a/builtin/bisect--helper.c
>> +++ b/builtin/bisect--helper.c
> [...]
>> +static int bisect_terms(struct bisect_terms *terms, const char **argv, int 
>> argc)
>> +{
>> + int i;
>> +
>> + if (get_terms(terms))
>> + return error(_("no terms defined"));
>> +
>> + if (argc > 1)
>> + return error(_("--bisect-term requires exactly one argument"));
>> +
>> + if (argc == 0)
>> + return !printf(_("Your current terms are %s for the old 
>> state\n"
>> +  "and %s for the new state.\n"),
>> +  terms->term_good, terms->term_bad);
>
> Same as in 1/8: you probably want "printf(...); return 0;" except there
> is a good reason.

No good reason. I will make the change.

>> +
>> + for (i = 0; i < argc; i++) {
>> + if (!strcmp(argv[i], "--term-good"))
>> + printf(_("%s\n"), terms->term_good);
>> + else if (!strcmp(argv[i], "--term-bad"))
>> + printf(_("%s\n"), terms->term_bad);
>
> The last two printfs: I think there is no point in translating "%s\n",
> so using "%s\n" instead of _("%s\n") looks more reasonable.

Also this probably does weird things with GETTEXT_POISON. I am
investigating what's happening as Martin pointed out in other thread..

>> + else
>> + error(_("BUG: invalid argument %s for 'git bisect 
>> terms'.\n"
>> +   "Supported options are: "
>> +   "--term-good|--term-old and "
>> +   "--term-bad|--term-new."), argv[i]);
>
> Should this be "return error(...)"?

Yeah. I missed this

>> + }
>> +
>> + return 0;
>> +}
>> +
>
> Stephan

Regards,
Pranit Bauva

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

[Johannes Schindelin]

Hi Junio,

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?

Ciao,
Dscho

Re: [PATCH v16 Part II 7/8] bisect--helper: `bisect_start` shell function partially in C

[Pranit Bauva]

Hey Stephan,

On Mon, Oct 30, 2017 at 10:21 PM, Stephan Beyer  wrote:
> Hi,
>
>> + return error(_("unrecognised option: '%s'"), arg);
>
> Please write "unrecogni_z_ed".
>
> Since the string for translation changed from
> "unrecognised option: '$arg'"
> to
> "unrecognised option: '%s'"
> anyway, it does not result in further work for the translators to
> correct it to
> "unrecognized option: '%s'"

Yeah Sure!

Regards,
Pranit Bauva

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

[Pranit Bauva]

Hey Stephan,

On Mon, Oct 30, 2017 at 6:52 PM, Stephan Beyer  wrote:
> Hi Pranit,
>> +static int bisect_reset(const char *commit)
>> +{
>> + struct strbuf branch = STRBUF_INIT;
>> +
>> + if (!commit) {
>> + if (strbuf_read_file(, git_path_bisect_start(), 0) < 1)
>> + return !printf(_("We are not bisecting.\n"));
>
> This is weird; I had to look up the return value of printf first before
> I could understand what you are doing ;) I think that it is meant as a
> shortcut for
>
> printf(_("We are not bisecting.\n"));
> return 0;
>
> but please also express it with these two lines. (Or what is the point
> of returning a non-zero value only in the case when nothing could be
> printed?)

I was just being a little lazy I suppose. I will stick to doing it in
two lines and avoiding fancy things.

Regards,
Pranit Bauva

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

[Pranit Bauva]

Hey Junio,

On Fri, Oct 27, 2017 at 11:10 PM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>> +static int bisect_reset(const char *commit)
>> +{
>> + struct strbuf branch = STRBUF_INIT;
>> +
>> + if (!commit) {
>> + if (strbuf_read_file(, git_path_bisect_start(), 0) < 1)
>> + return !printf(_("We are not bisecting.\n"));
>> + strbuf_rtrim();
>> + } else {
>> + struct object_id oid;
>> +
>> + if (get_oid_commit(commit, ))
>> + return error(_("'%s' is not a valid commit"), commit);
>> + strbuf_addstr(, commit);
>
> The original checks "test -s BISECT_START" and complains, even when
> an explicit commit is given.  With this change, when the user is not
> bisecting, giving "git bisect reset master" goes ahead---it is
> likely that BISECT_HEAD does not exist and we may hit "Could not
> check out" error, but if BISECT_HEAD is left behind from a previous
> run (which is likely completely unrelated to whatever the user
> currently is doing), we'd end up doing quite a random thing, no?

Yes. Thanks for mentioning this point. I don't quite remember things
right now about what made me do this change. There might have been
something which had made me do this change because this isn't just a
silly mistake. Any which ways, I couldn't recollect the reason (should
be more careful to put code comments).

>> + }
>> +
>> + if (!file_exists(git_path_bisect_head())) {
>> + struct argv_array argv = ARGV_ARRAY_INIT;
>> +
>> + argv_array_pushl(, "checkout", branch.buf, "--", NULL);
>> + if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
>> + error(_("Could not check out original HEAD '%s'. Try "
>> + "'git bisect reset '."), branch.buf);
>> + strbuf_release();
>> + argv_array_clear();
>> + return -1;
>
> How does this return value affect the value eventually given to
> exit(3), called by somewhere in git.c that called this function?
>
> The call graph would be
>
> common-main.c::main()
> -> git.c::cmd_main()
>-> handle_builtin()
>   . exit(run_builtin())
>   -> run_builtin()
>  . status = p->fn()
>  -> cmd_bisect__helper()
> . return bisect_reset()
> -> bisect_reset()
>. return -1
>  . if (status) return status;
>
> So the -1 is returned throughout the callchain and exit(3) ends up
> getting it---which is not quite right.  We shouldn't be giving
> negative value to exit(3).  bisect_clean_state() and other helper
> functions may already share the same issue.

I had totally missed that exit() takes only single byte value and thus
only positive integers. I think changing it to "return 1;" will do.
There are a few places in the previous series which use "return -1;"
which would need to be changed. I will resend that series.

>> + }
>> + argv_array_clear();
>> + }
>> +
>> + strbuf_release();
>> + return bisect_clean_state();
>> +}

Regards,
Pranit Bauva

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

[Johannes Schindelin]

Hi Junio,

On Mon, 30 Oct 2017, Junio C Hamano wrote:

> * cc/git-packet-pm (2017-10-30) 7 commits
>  - fixup! Git/Packet.pm: extract parts of t0021/rot13-filter.pl for reuse

I am really terribly sorry for the breakage I introduced here. Junio,
would you mind amending this commit by deleting the "SVN/" in
"Git/SVN/Packet"?

Thanks a lot,
Dscho

[PATCH v5 3/4] status: document options to show matching ignored files

[jameson . miller81]

From: Jameson Miller 

Signed-off-by: Jameson Miller 
---
 Documentation/git-status.txt  | 21 +-
 Documentation/technical/api-directory-listing.txt | 27 +++
 2 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 9f3a78a36c..fc282e0a92 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -97,8 +97,27 @@ configuration variable documented in linkgit:git-config[1].
(and suppresses the output of submodule summaries when the config option
`status.submoduleSummary` is set).
 
---ignored::
+--ignored[=]::
Show ignored files as well.
++
+The mode parameter is used to specify the handling of ignored files.
+It is optional: it defaults to 'traditional'.
++
+The possible options are:
++
+   - 'traditional' - Shows ignored files and directories, unless
+ --untracked-files=all is specifed, in which case
+ individual files in ignored directories are
+ displayed.
+   - 'no'  - Show no ignored files.
+   - 'matching'- Shows ignored files and directories matching an
+ ignore pattern.
++
+When 'matching' mode is specified, paths that explicity match an
+ignored pattern are shown. If a directory matches an ignore pattern,
+then it is shown, but not paths contained in the ignored directory. If
+a directory does not match an ignore pattern, but all contents are
+ignored, then the directory is not shown, but all contents are shown.
 
 -z::
Terminate entries with NUL, instead of LF.  This implies
diff --git a/Documentation/technical/api-directory-listing.txt 
b/Documentation/technical/api-directory-listing.txt
index 6c77b4920c..7fae00f44f 100644
--- a/Documentation/technical/api-directory-listing.txt
+++ b/Documentation/technical/api-directory-listing.txt
@@ -22,16 +22,20 @@ The notable options are:
 
 `flags`::
 
-   A bit-field of options (the `*IGNORED*` flags are mutually exclusive):
+   A bit-field of options:
 
 `DIR_SHOW_IGNORED`:::
 
-   Return just ignored files in `entries[]`, not untracked files.
+   Return just ignored files in `entries[]`, not untracked
+   files. This flag is mutually exclusive with
+   `DIR_SHOW_IGNORED_TOO`.
 
 `DIR_SHOW_IGNORED_TOO`:::
 
-   Similar to `DIR_SHOW_IGNORED`, but return ignored files in `ignored[]`
-   in addition to untracked files in `entries[]`.
+   Similar to `DIR_SHOW_IGNORED`, but return ignored files in
+   `ignored[]` in addition to untracked files in
+   `entries[]`. This flag is mutually exclusive with
+   `DIR_SHOW_IGNORED`.
 
 `DIR_KEEP_UNTRACKED_CONTENTS`:::
 
@@ -39,6 +43,21 @@ The notable options are:
untracked contents of untracked directories are also returned in
`entries[]`.
 
+`DIR_SHOW_IGNORED_TOO_MODE_MATCHING`:::
+
+   Only has meaning if `DIR_SHOW_IGNORED_TOO` is also set; if
+   this is set, returns ignored files and directories that match
+   an exclude pattern. If a directory matches an exclude pattern,
+   then the directory is returned and the contained paths are
+   not. A directory that does not match an exclude pattern will
+   not be returned even if all of its contents are ignored. In
+   this case, the contents are returned as individual entries.
++
+If this is set, files and directories that explicity match an ignore
+pattern are reported. Implicity ignored directories (directories that
+do not match an ignore pattern, but whose contents are all ignored)
+are not reported, instead all of the contents are reported.
+
 `DIR_COLLECT_IGNORED`:::
 
Special mode for git-add. Return ignored files in `ignored[]` and
-- 
2.13.6

[PATCH v5 1/4] status: add option to show ignored files differently

[jameson . miller81]

From: Jameson Miller 

Teach the status command more flexibility in how ignored files are
reported. Currently, the reporting of ignored files and untracked
files are linked. You cannot control how ignored files are reported
independently of how untracked files are reported (i.e. `all` vs
`normal`). This makes it impossible to show untracked files with the
`all` option, but show ignored files with the `normal` option.

This work 1) adds the ability to control the reporting of ignored
files independently of untracked files and 2) introduces the concept
of status reporting ignored paths that explicitly match an ignored
pattern. There are 2 benefits to these changes: 1) if a consumer needs
all untracked files but not all ignored files, there is a performance
benefit to not scanning all contents of an ignored directory and 2)
returning ignored files that explicitly match a path allow a consumer
to make more informed decisions about when a status result might be
stale.

This commit implements --ignored=matching with --untracked-files=all.
The following commit will implement --ignored=matching with
--untracked=files=normal.

As an example of where this flexibility could be useful is that our
application (Visual Studio) runs the status command and presents the
output. It shows all untracked files individually (e.g. using the
'--untracked-files==all' option), and would like to know about which
paths are ignored. It uses information about ignored paths to make
decisions about when the status result might have changed.
Additionally, many projects place build output into directories inside
a repository's working directory (e.g. in "bin/" and "obj/"
directories). Normal usage is to explicitly ignore these 2 directory
names in the .gitignore file (rather than or in addition to the *.obj
pattern).If an application could know that these directories are
explicitly ignored, it could infer that all contents are ignored as
well and make better informed decisions about files in these
directories. It could infer that any changes under these paths would
not affect the output of status. Additionally, there can be a
significant performance benefit by avoiding scanning through ignored
directories.

When status is set to report matching ignored files, it has the
following behavior. Ignored files and directories that explicitly
match an exclude pattern are reported. If an ignored directory matches
an exclude pattern, then the path of the directory is returned. If a
directory does not match an exclude pattern, but all of its contents
are ignored, then the contained files are reported instead of the
directory.

Signed-off-by: Jameson Miller 
---
 builtin/commit.c | 31 +--
 dir.c| 24 
 dir.h|  3 ++-
 wt-status.c  | 11 ---
 wt-status.h  |  8 +++-
 5 files changed, 66 insertions(+), 11 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d75b3805ea..98d84d0277 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -118,7 +118,7 @@ static int edit_flag = -1; /* unspecified */
 static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
 static int config_commit_verbose = -1; /* unspecified */
 static int no_post_rewrite, allow_empty_message;
-static char *untracked_files_arg, *force_date, *ignore_submodule_arg;
+static char *untracked_files_arg, *force_date, *ignore_submodule_arg, 
*ignored_arg;
 static char *sign_commit;
 
 /*
@@ -139,7 +139,7 @@ static const char *cleanup_arg;
 static enum commit_whence whence;
 static int sequencer_in_use;
 static int use_editor = 1, include_status = 1;
-static int show_ignored_in_status, have_option_m;
+static int have_option_m;
 static struct strbuf message = STRBUF_INIT;
 
 static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
@@ -1075,6 +1075,19 @@ static const char *find_author_by_nickname(const char 
*name)
die(_("--author '%s' is not 'Name ' and matches no existing 
author"), name);
 }
 
+static void handle_ignored_arg(struct wt_status *s)
+{
+   if (!ignored_arg)
+   ; /* default already initialized */
+   else if (!strcmp(ignored_arg, "traditional"))
+   s->show_ignored_mode = SHOW_TRADITIONAL_IGNORED;
+   else if (!strcmp(ignored_arg, "no"))
+   s->show_ignored_mode = SHOW_NO_IGNORED;
+   else if (!strcmp(ignored_arg, "matching"))
+   s->show_ignored_mode = SHOW_MATCHING_IGNORED;
+   else
+   die(_("Invalid ignored mode '%s'"), ignored_arg);
+}
 
 static void handle_untracked_files_arg(struct wt_status *s)
 {
@@ -1363,8 +1376,10 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
  N_("mode"),
  N_("show untracked files, optional modes: all, normal, no. 
(Default: all)"),
  PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
-   OPT_BOOL(0, "ignored", 

[PATCH v5 4/4] status: test ignored modes

[jameson . miller81]

From: Jameson Miller 

Add tests around status reporting ignord files that match an exclude
pattern for both --untracked-files=normal and --untracked-files=all.

Signed-off-by: Jameson Miller 
---
 t/t7521-ignored-mode.sh | 233 
 1 file changed, 233 insertions(+)
 create mode 100755 t/t7521-ignored-mode.sh

diff --git a/t/t7521-ignored-mode.sh b/t/t7521-ignored-mode.sh
new file mode 100755
index 00..91790943c3
--- /dev/null
+++ b/t/t7521-ignored-mode.sh
@@ -0,0 +1,233 @@
+#!/bin/sh
+
+test_description='git status ignored modes'
+
+. ./test-lib.sh
+
+test_expect_success 'setup initial commit and ignore file' '
+   cat >.gitignore <<-\EOF &&
+   *.ign
+   ignored_dir/
+   !*.unignore
+   EOF
+   git add . &&
+   git commit -m "Initial commit"
+'
+
+test_expect_success 'Verify behavior of status on directories with ignored 
files' '
+   test_when_finished "git clean -fdx" &&
+   cat >expect <<-\EOF &&
+   ? expect
+   ? output
+   ! dir/ignored/ignored_1.ign
+   ! dir/ignored/ignored_2.ign
+   ! ignored/ignored_1.ign
+   ! ignored/ignored_2.ign
+   EOF
+
+   mkdir -p ignored dir/ignored &&
+   touch ignored/ignored_1.ign ignored/ignored_2.ign \
+   dir/ignored/ignored_1.ign dir/ignored/ignored_2.ign &&
+
+   git status --porcelain=v2 --ignored=matching --untracked-files=all 
>output &&
+   test_i18ncmp expect output
+'
+
+test_expect_success 'Verify status behavior on directory with tracked & 
ignored files' '
+   test_when_finished "git clean -fdx && git reset HEAD~1 --hard" &&
+   cat >expect <<-\EOF &&
+   ? expect
+   ? output
+   ! dir/tracked_ignored/ignored_1.ign
+   ! dir/tracked_ignored/ignored_2.ign
+   ! tracked_ignored/ignored_1.ign
+   ! tracked_ignored/ignored_2.ign
+   EOF
+
+   mkdir -p tracked_ignored dir/tracked_ignored &&
+   touch tracked_ignored/tracked_1 tracked_ignored/tracked_2 \
+   tracked_ignored/ignored_1.ign tracked_ignored/ignored_2.ign \
+   dir/tracked_ignored/tracked_1 dir/tracked_ignored/tracked_2 \
+   dir/tracked_ignored/ignored_1.ign 
dir/tracked_ignored/ignored_2.ign &&
+
+   git add tracked_ignored/tracked_1 tracked_ignored/tracked_2 \
+   dir/tracked_ignored/tracked_1 dir/tracked_ignored/tracked_2 &&
+   git commit -m "commit tracked files" &&
+
+   git status --porcelain=v2 --ignored=matching --untracked-files=all 
>output &&
+   test_i18ncmp expect output
+'
+
+test_expect_success 'Verify status behavior on directory with untracked and 
ignored files' '
+   test_when_finished "git clean -fdx" &&
+   cat >expect <<-\EOF &&
+   ? dir/untracked_ignored/untracked_1
+   ? dir/untracked_ignored/untracked_2
+   ? expect
+   ? output
+   ? untracked_ignored/untracked_1
+   ? untracked_ignored/untracked_2
+   ! dir/untracked_ignored/ignored_1.ign
+   ! dir/untracked_ignored/ignored_2.ign
+   ! untracked_ignored/ignored_1.ign
+   ! untracked_ignored/ignored_2.ign
+   EOF
+
+   mkdir -p untracked_ignored dir/untracked_ignored &&
+   touch untracked_ignored/untracked_1 untracked_ignored/untracked_2 \
+   untracked_ignored/ignored_1.ign untracked_ignored/ignored_2.ign 
\
+   dir/untracked_ignored/untracked_1 
dir/untracked_ignored/untracked_2 \
+   dir/untracked_ignored/ignored_1.ign 
dir/untracked_ignored/ignored_2.ign &&
+
+   git status --porcelain=v2 --ignored=matching --untracked-files=all 
>output &&
+   test_i18ncmp expect output
+'
+
+test_expect_success 'Verify status matching ignored files on ignored 
directory' '
+   test_when_finished "git clean -fdx" &&
+   cat >expect <<-\EOF &&
+   ? expect
+   ? output
+   ! ignored_dir/
+   EOF
+
+   mkdir ignored_dir &&
+   touch ignored_dir/ignored_1 ignored_dir/ignored_2 \
+   ignored_dir/ignored_1.ign ignored_dir/ignored_2.ign &&
+
+   git status --porcelain=v2 --ignored=matching --untracked-files=all 
>output &&
+   test_i18ncmp expect output
+'
+
+test_expect_success 'Verify status behavior on ignored directory containing 
tracked file' '
+   test_when_finished "git clean -fdx && git reset HEAD~1 --hard" &&
+   cat >expect <<-\EOF &&
+   ? expect
+   ? output
+   ! ignored_dir/ignored_1
+   ! ignored_dir/ignored_1.ign
+   ! ignored_dir/ignored_2
+   ! ignored_dir/ignored_2.ign
+   EOF
+
+   mkdir ignored_dir &&
+   touch ignored_dir/ignored_1 ignored_dir/ignored_2 \
+   ignored_dir/ignored_1.ign ignored_dir/ignored_2.ign \
+   ignored_dir/tracked &&
+   git add -f ignored_dir/tracked &&
+   git commit -m "Force add file in ignored directory" &&
+   git status --porcelain=v2 --ignored=matching 

[PATCH v5 2/4] status: report matching ignored and normal untracked

[jameson . miller81]

From: Jameson Miller 

Teach status command to handle `--ignored=matching` with
`--untracked-files=normal`

Signed-off-by: Jameson Miller 
---
 dir.c | 20 ++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/dir.c b/dir.c
index b9af87eca9..20457724c0 100644
--- a/dir.c
+++ b/dir.c
@@ -1585,6 +1585,7 @@ static enum path_treatment treat_one_path(struct 
dir_struct *dir,
 {
int exclude;
int has_path_in_index = !!index_file_exists(istate, path->buf, 
path->len, ignore_case);
+   enum path_treatment path_treatment;
 
if (dtype == DT_UNKNOWN)
dtype = get_dtype(de, istate, path->buf, path->len);
@@ -1631,8 +1632,23 @@ static enum path_treatment treat_one_path(struct 
dir_struct *dir,
return path_none;
case DT_DIR:
strbuf_addch(path, '/');
-   return treat_directory(dir, istate, untracked, path->buf, 
path->len,
-  baselen, exclude, pathspec);
+   path_treatment = treat_directory(dir, istate, untracked,
+path->buf, path->len,
+baselen, exclude, pathspec);
+   /*
+* If 1) we only want to return directories that
+* match an exclude pattern and 2) this directory does
+* not match an exclude pattern but all of its
+* contents are excluded, then indicate that we should
+* recurse into this directory (instead of marking the
+* directory itself as an ignored path).
+*/
+   if (!exclude &&
+   path_treatment == path_excluded &&
+   (dir->flags & DIR_SHOW_IGNORED_TOO) &&
+   (dir->flags & DIR_SHOW_IGNORED_TOO_MODE_MATCHING))
+   return path_recurse;
+   return path_treatment;
case DT_REG:
case DT_LNK:
return exclude ? path_excluded : path_untracked;
-- 
2.13.6

[PATCH v5 0/4] status: add option to show ignored files differently

[jameson . miller81]

From: Jameson Miller 

Previous patch series can be found:
https://public-inbox.org/git/20171023170534.157740-1-jam...@microsoft.com/

Only difference from previous version is to update the commit author
email to match corporate email address.

Jameson Miller (4):
  status: add option to show ignored files differently
  status: report matching ignored and normal untracked
  status: document options to show matching ignored files
  status: test ignored modes

 Documentation/git-status.txt  |  21 +-
 Documentation/technical/api-directory-listing.txt |  27 ++-
 builtin/commit.c  |  31 ++-
 dir.c |  44 +++-
 dir.h |   3 +-
 t/t7521-ignored-mode.sh   | 233 ++
 wt-status.c   |  11 +-
 wt-status.h   |   8 +-
 8 files changed, 360 insertions(+), 18 deletions(-)
 create mode 100755 t/t7521-ignored-mode.sh

-- 
2.13.6

[PATCH 2/2] wincred: handle empty username/password correctly

[Johannes Schindelin]

From: =?UTF-8?q?Jakub=20Bere=C5=BCa=C5=84ski?= 

Empty (length 0) usernames and/or passwords, when saved in the Windows
Credential Manager, come back as null when reading the credential.

One use case for such empty credentials is with NTLM authentication, where
empty username and password instruct libcurl to authenticate using the
credentials of the currently logged-on user (single sign-on).

When locating the relevant credentials, make empty username match null.
When outputting the credentials, handle nulls correctly.

Signed-off-by: Jakub Bereżański 
---
 contrib/credential/wincred/git-credential-wincred.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/contrib/credential/wincred/git-credential-wincred.c 
b/contrib/credential/wincred/git-credential-wincred.c
index 006134043a4..86518cd93d9 100644
--- a/contrib/credential/wincred/git-credential-wincred.c
+++ b/contrib/credential/wincred/git-credential-wincred.c
@@ -94,6 +94,12 @@ static WCHAR *wusername, *password, *protocol, *host, *path, 
target[1024];
 static void write_item(const char *what, LPCWSTR wbuf, int wlen)
 {
char *buf;
+
+   if (!wbuf || !wlen) {
+   printf("%s=\n", what);
+   return;
+   }
+
int len = WideCharToMultiByte(CP_UTF8, 0, wbuf, wlen, NULL, 0, NULL,
FALSE);
buf = xmalloc(len);
@@ -160,7 +166,7 @@ static int match_part_last(LPCWSTR *ptarget, LPCWSTR want, 
LPCWSTR delim)
 static int match_cred(const CREDENTIALW *cred)
 {
LPCWSTR target = cred->TargetName;
-   if (wusername && wcscmp(wusername, cred->UserName))
+   if (wusername && wcscmp(wusername, cred->UserName ? cred->UserName : 
L""))
return 0;
 
return match_part(, L"git", L":") &&
@@ -183,7 +189,7 @@ static void get_credential(void)
for (i = 0; i < num_creds; ++i)
if (match_cred(creds[i])) {
write_item("username", creds[i]->UserName,
-   wcslen(creds[i]->UserName));
+   creds[i]->UserName ? wcslen(creds[i]->UserName) 
: 0);
write_item("password",
(LPCWSTR)creds[i]->CredentialBlob,
creds[i]->CredentialBlobSize / sizeof(WCHAR));
-- 
2.15.0.windows.1

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

[Stefan Beller]

On Fri, Oct 27, 2017 at 10:07 AM, Stefan Beller  wrote:
>> Let's do this bit-shuffling as a preliminary clean-up.
>
> These 2 patches can go on top of that as well.

Actually these textually do not conflict with your patch,
and they can be picked independently, e.g. they could
go on top of sb/diff-color-moved-use-xdl-recmatch
as a diff cleanup.

(I note this as you regard your patches as a lunch time hack
in the cooking email; I am serious about these patches though.)

Thanks,
Stefan

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

[Johannes Schindelin]

From: =?UTF-8?q?Jakub=20Bere=C5=BCa=C5=84ski?= 

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.

Signed-off-by: Jakub Bereżański 
---
 t/lib-credential.sh | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/t/lib-credential.sh b/t/lib-credential.sh
index d8e41f7ddd1..937b831ea67 100755
--- a/t/lib-credential.sh
+++ b/t/lib-credential.sh
@@ -44,6 +44,7 @@ helper_test_clean() {
reject $1 https example.com user2
reject $1 http path.tld user
reject $1 https timeout.tld user
+   reject $1 https sso.tld
 }
 
 reject() {
@@ -250,6 +251,24 @@ helper_test() {
password=pass2
EOF
'
+
+   test_expect_success "helper ($HELPER) can store empty username" '
+   check approve $HELPER <<-\EOF &&
+   protocol=https
+   host=sso.tld
+   username=
+   password=
+   EOF
+   check fill $HELPER <<-\EOF
+   protocol=https
+   host=sso.tld
+   --
+   protocol=https
+   host=sso.tld
+   username=
+   password=
+   EOF
+   '
 }
 
 helper_test_timeout() {
-- 
2.15.0.windows.1

[PATCH 0/2] wincred: learn to handle "empty credentials"

[Johannes Schindelin]

As we learned some time ago, NTLM authentication happens by passing
"empty credentials", i.e. 0-length usernames and passwords.

However, when saved in the Windows Credential Manager, such usernames
and passwords come back as null when reading the credential. Let's
handle this.

This patch series is a tender four years old and has been simmering in
Git for Windows since version v1.8.4, so it is most likely mature enough
(even at that young age) to enter core Git.

Note: these days, Git for Windows prefers to use the Git Credential
Manager for Windows instead, but the wincred code is still carried in
Git's contrib/ and should be fixed there, too.


Jakub Bereżański (2):
  t0302: check helper can handle empty credentials
  wincred: handle empty username/password correctly

 contrib/credential/wincred/git-credential-wincred.c | 10 --
 t/lib-credential.sh | 19 +++
 2 files changed, 27 insertions(+), 2 deletions(-)


base-commit: cb5918aa0d50f50e83787f65c2ddc3dcb10159fe
Published-As: 
https://github.com/dscho/git/releases/tag/jberezanski/wincred-sso-r2-v1
Fetch-It-Via: git fetch https://github.com/dscho/git 
jberezanski/wincred-sso-r2-v1
-- 
2.15.0.windows.1

[PATCH] mingw: include the full version information in the resources

[Johannes Schindelin]

This fixes https://github.com/git-for-windows/git/issues/723

Signed-off-by: Johannes Schindelin 
---

This patch has been carried in Git for Windows in this form since
Git for Windows v2.11.0(2). It seems to be ready for prime time.

I could just imagine *one* change: to use

tr -d 0-9 ' '

instead of

tr '.a-zA-Z-' ' '

However, I am reluctant to do that, as any character that is not
caught by this pattern would be indicative of something really
funny going on.

Plus, I have had really bad experience with changing a well-tested
patch series just to get it into core Git, only to see an
immediate breakage introduced to appease reviews. Therefore, this
time around I will need to be convinced quite a bit more if I am to
risk breakages.

 Makefile | 3 ++-
 git.rc   | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index cd75985991f..ee9d5eb11ee 100644
--- a/Makefile
+++ b/Makefile
@@ -1940,7 +1940,8 @@ $(SCRIPT_LIB) : % : %.sh GIT-SCRIPT-DEFINES
 
 git.res: git.rc GIT-VERSION-FILE
$(QUIET_RC)$(RC) \
- $(join -DMAJOR= -DMINOR=, $(wordlist 1,2,$(subst -, ,$(subst ., 
,$(GIT_VERSION) \
+ $(join -DMAJOR= -DMINOR= -DMICRO= -DPATCHLEVEL=, $(wordlist 1, 4, \
+   $(shell echo $(GIT_VERSION) 0 0 0 0 | tr '.a-zA-Z-' ' '))) \
  -DGIT_VERSION="\\\"$(GIT_VERSION)\\\"" -i $< -o $@
 
 # This makes sure we depend on the NO_PERL setting itself.
diff --git a/git.rc b/git.rc
index 33aafb786cf..49002e0d541 100644
--- a/git.rc
+++ b/git.rc
@@ -1,6 +1,6 @@
 1 VERSIONINFO
-FILEVERSION MAJOR,MINOR,0,0
-PRODUCTVERSION  MAJOR,MINOR,0,0
+FILEVERSION MAJOR,MINOR,MICRO,PATCHLEVEL
+PRODUCTVERSION  MAJOR,MINOR,MICRO,PATCHLEVEL
 BEGIN
   BLOCK "StringFileInfo"
   BEGIN

base-commit: cb5918aa0d50f50e83787f65c2ddc3dcb10159fe
-- 
2.15.0.windows.1

Published-As: https://github.com/dscho/git/releases/tag/resource-version-v1
Fetch-It-Via: git fetch https://github.com/dscho/git resource-version-v1

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

[Johannes Schindelin]

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 
---
 Documentation/git.txt | 17 +
 1 file changed, 17 insertions(+)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 7a1d629ca06..10a98603b39 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -709,6 +709,23 @@ of clones and fetches.
the background which do not want to cause lock contention with
other operations on the repository.  Defaults to `1`.
 
+`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.
+
 Discussion[[Discussion]]
 
 
-- 
2.15.0.windows.1

[PATCH 2/3] mingw: special-case GIT_REDIRECT_STDERR=2>&1

[Johannes Schindelin]

The "2>&1" notation in POSIX shells implies that stderr is redirected to
stdout. Let's special-case this value for the environment variable
GIT_REDIRECT_STDERR to allow writing to the same destination as stdout.

The functionality was suggested by Jeff Hostetler.

Signed-off-by: Johannes Schindelin 
---
 compat/mingw.c  | 15 +++
 t/t0001-init.sh |  8 +++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 6c6c7795a70..2d44d21aca8 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2160,6 +2160,21 @@ static void maybe_redirect_std_handle(const wchar_t 
*key, DWORD std_id, int fd,
CloseHandle(handle);
return;
}
+   if (std_id == STD_ERROR_HANDLE && !wcscmp(buf, L"2>&1")) {
+   handle = GetStdHandle(STD_OUTPUT_HANDLE);
+   if (handle == INVALID_HANDLE_VALUE) {
+   close(fd);
+   handle = GetStdHandle(std_id);
+   if (handle != INVALID_HANDLE_VALUE)
+   CloseHandle(handle);
+   } else {
+   int new_fd = _open_osfhandle((intptr_t)handle, 
O_BINARY);
+   SetStdHandle(std_id, handle);
+   dup2(new_fd, fd);
+   /* do *not* close the new_fd: that would close stdout */
+   }
+   return;
+   }
handle = CreateFileW(buf, desired_access, 0, NULL, create_flag,
 flags, NULL);
if (handle != INVALID_HANDLE_VALUE) {
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 0fd2fc45385..c413bff9cf1 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -456,7 +456,13 @@ test_expect_success 're-init from a linked worktree' '
 test_expect_success MINGW 'redirect std handles' '
GIT_REDIRECT_STDOUT=output.txt git rev-parse --git-dir &&
test .git = "$(cat output.txt)" &&
-   test -z "$(GIT_REDIRECT_STDOUT=off git rev-parse --git-dir)"
+   test -z "$(GIT_REDIRECT_STDOUT=off git rev-parse --git-dir)" &&
+   test_must_fail env \
+   GIT_REDIRECT_STDOUT=output.txt \
+   GIT_REDIRECT_STDERR="2>&1" \
+   git rev-parse --git-dir --verify refs/invalid &&
+   printf ".git\nfatal: Needed a single revision\n" >expect &&
+   test_cmp expect output.txt
 '
 
 test_done
-- 
2.15.0.windows.1

[PATCH 1/3] mingw: add experimental feature to redirect standard handles

[Johannes Schindelin]

On Windows, file handles need to be marked inheritable when they need to
be used as standard input/output/error handles for a newly spawned
process. The problem with that, of course, is that the "inheritable" flag
is global and therefore can wreak havoc with highly multi-threaded
applications: other spawned processes will *also* inherit those file
handles, despite having *other* input/output/error handles, and never
close the former handles because they do not know about them.

Let's introduce a set of environment variables (GIT_REDIRECT_STDIN and
friends) that point to files, or even better, named pipes and that are
used by the spawned Git process. This helps work around above-mentioned
issue: those named pipes will be opened in a non-inheritable way upon
startup, and no handles are passed around.

Signed-off-by: Johannes Schindelin 
---
 compat/mingw.c  | 43 +++
 t/t0001-init.sh |  6 ++
 2 files changed, 49 insertions(+)

diff --git a/compat/mingw.c b/compat/mingw.c
index 8b6fa0db446..6c6c7795a70 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2139,6 +2139,47 @@ static char *wcstoutfdup_startup(char *buffer, const 
wchar_t *wcs, size_t len)
return memcpy(malloc_startup(len), buffer, len);
 }
 
+static void maybe_redirect_std_handle(const wchar_t *key, DWORD std_id, int fd,
+ DWORD desired_access, DWORD flags)
+{
+   DWORD create_flag = fd ? OPEN_ALWAYS : OPEN_EXISTING;
+   wchar_t buf[MAX_PATH];
+   DWORD max = ARRAY_SIZE(buf);
+   HANDLE handle;
+   DWORD ret = GetEnvironmentVariableW(key, buf, max);
+
+   if (!ret || ret >= max)
+   return;
+
+   /* make sure this does not leak into child processes */
+   SetEnvironmentVariableW(key, NULL);
+   if (!wcscmp(buf, L"off")) {
+   close(fd);
+   handle = GetStdHandle(std_id);
+   if (handle != INVALID_HANDLE_VALUE)
+   CloseHandle(handle);
+   return;
+   }
+   handle = CreateFileW(buf, desired_access, 0, NULL, create_flag,
+flags, NULL);
+   if (handle != INVALID_HANDLE_VALUE) {
+   int new_fd = _open_osfhandle((intptr_t)handle, O_BINARY);
+   SetStdHandle(std_id, handle);
+   dup2(new_fd, fd);
+   close(new_fd);
+   }
+}
+
+static void maybe_redirect_std_handles(void)
+{
+   maybe_redirect_std_handle(L"GIT_REDIRECT_STDIN", STD_INPUT_HANDLE, 0,
+ GENERIC_READ, FILE_ATTRIBUTE_NORMAL);
+   maybe_redirect_std_handle(L"GIT_REDIRECT_STDOUT", STD_OUTPUT_HANDLE, 1,
+ GENERIC_WRITE, FILE_ATTRIBUTE_NORMAL);
+   maybe_redirect_std_handle(L"GIT_REDIRECT_STDERR", STD_ERROR_HANDLE, 2,
+ GENERIC_WRITE, FILE_FLAG_NO_BUFFERING);
+}
+
 void mingw_startup(void)
 {
int i, maxlen, argc;
@@ -2146,6 +2187,8 @@ void mingw_startup(void)
wchar_t **wenv, **wargv;
_startupinfo si;
 
+   maybe_redirect_std_handles();
+
/* get wide char arguments and environment */
si.newmode = 0;
if (__wgetmainargs(, , , _CRT_glob, ) < 0)
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 86c1a51654f..0fd2fc45385 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -453,4 +453,10 @@ test_expect_success 're-init from a linked worktree' '
)
 '
 
+test_expect_success MINGW 'redirect std handles' '
+   GIT_REDIRECT_STDOUT=output.txt git rev-parse --git-dir &&
+   test .git = "$(cat output.txt)" &&
+   test -z "$(GIT_REDIRECT_STDOUT=off git rev-parse --git-dir)"
+'
+
 test_done
-- 
2.15.0.windows.1

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

[Johannes Schindelin]

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.

Once it has been reviewed enough, I will gladly remove the
"experimental" adjectives and warnings.


Johannes Schindelin (3):
  mingw: add experimental feature to redirect standard handles
  mingw: special-case GIT_REDIRECT_STDERR=2>&1
  mingw: document the experimental standard handle redirection

 Documentation/git.txt | 17 +++
 compat/mingw.c| 58 +++
 t/t0001-init.sh   | 12 +++
 3 files changed, 87 insertions(+)


base-commit: cb5918aa0d50f50e83787f65c2ddc3dcb10159fe
Published-As: https://github.com/dscho/git/releases/tag/redirect-std-handles-v1
Fetch-It-Via: git fetch https://github.com/dscho/git redirect-std-handles-v1
-- 
2.15.0.windows.1

Re: Why does fetch-pack not works over http?

[Alvaro del Castillo]

Hi Andreas,

El vie, 27-10-2017 a las 14:33 +0200, Andreas Schwab escribió:
> On Okt 27 2017, Alvaro del Castillo  wrote:
> 
> > 
> > We're wondering why "fetch-pack" (when is running from the command
> > line) doesn't handle "https://; protocol. It only works with
> > "git://".
> > 
> > For instance, this doesn't work:
> > 
> > $ git fetch-pack https://github.com/git/git refs/heads/master
> > fatal: I don't handle protocol 'https'
> > 
> > while this does:
> > 
> > $ git fetch-pack git://github.com/git/git refs/heads/master
> > 
> > The funny thing is that under the hood, "fetch" calls "fetch-pack"
> > using "https" procotol. Example of a trace below:
> > 
> > 12:03:07.512558 git.c:344   trace: built-in: git
> > 'fetch-
> > pack' '--stateless-rpc' '--stdin' '--lock-pack' '--thin' 'https://g
> > ithu
> > b.com/git/git/'
> With --stateless-rpc, fetch-pack doesn't do the connect itself, but
> expects the caller having set up a pipe to it.  The URL is then
> actually
> ignored.

Ok, I have understood that reading the git code but, is it possible to
create this pipe using command line? Or is that something designed to
be used inside the git execution?

Thanks!

> 
> Andreas.
> 

-- 
Alvaro del Castillo San Félix
a...@bitergia.com - Chief Technical Officer (CTO)
http://www.bitergia.com
"Software metrics for your peace of mind"

Re: [PATCH v16 Part II 7/8] bisect--helper: `bisect_start` shell function partially in C

[Stephan Beyer]

Hi,

> + return error(_("unrecognised option: '%s'"), arg);

Please write "unrecogni_z_ed".

Since the string for translation changed from
"unrecognised option: '$arg'"
to
"unrecognised option: '%s'"
anyway, it does not result in further work for the translators to
correct it to
"unrecognized option: '%s'"

Stephan

Re: [PATCH v2 0/4] Hash Abstraction

[Stefan Beller]

On Sat, Oct 28, 2017 at 11:12 AM, brian m. carlson
 wrote:
> This is a series proposing a basic abstraction for hash functions.
>
> As we get closer to converting the remainder of the codebase to use
> struct object_id, we should think about the design we want our hash
> function abstraction to take.  This series is a proposal for one idea.
> Input on any aspect of this proposal is welcome.

I like the series/idea.

> This series exposes a struct git_hash_algo that contains basic
> information about a given hash algorithm that distinguishes it from
> other algorithms: name, identifiers, lengths, implementing functions,
> and empty tree and blob constants.  It also exposes an array of hash
> algorithms, and a constant for indexing them.
>
> The series also demonstrates a simple conversion using the abstraction
> over empty blob and tree values.

That looks good, though I wonder if we'll have to give a way a little
performance during the transition period (or even indefinitely, as the
hash abstraction won't go away; we'd rather want to keep it in place
long term). I guess we can measure after all is said and done,
no big deal.

>
> In order to avoid conflicting with the struct repository work and with
> the goal of avoiding global variables as much as possible, I've pushed
> the hash algorithm into struct repository and exposed it via a #define.

If you are referring to that long series[1] that Jonathan and I were working
on, then no worries.  Unfortunately that series got some delays, but I rebased
its prepared successors (of over 100 patches) on Friday and it was surprisingly
easy to do so; just tedious.

[1] 
https://public-inbox.org/git/20170830064634.ga153...@aiede.mtv.corp.google.com/

>
> I propose this series now as it will inform the way we go about
> converting other parts of the codebase, especially some of the pack
> algorithms.

Thanks for doing this now.

> Because we share some hash computation code between pack
> checksums and object hashing, we need to decide whether to expose pack
> checksums as struct object_id, even though they are technically not
> object IDs.

I think we used sha1 as checksums, because it was a hash function easily
accessible, not because its other properties were the best to do its job.
So I am undecided if we want to just "keep the same hash function for
checksum as well as object hashing" or pickup another checksum, that
actually *only* does checksumming (we don't need the cryptographic
properties, such that an easy hash [such as crc, djb, fnv or murmur]
would do; raw speed, barely protecting the pack file against bit flips).

For the sake of symmetry I tend to go with the former, i.e use the object
hash function for pack files, too.

>  Furthermore, if we end up needing to stuff an algorithm
> value into struct object_id, we'll no longer be able to directly
> reference object IDs in a pack without a copy.
>
> I've updated this series in some significant ways to reflect and better
> implement the transition plan as it's developed.  If there are ways
> in which this series (or future series) can converge better on the
> transition plan, that input would be valuable.
>
> This series is available from the usual places as branch hash-struct,
> based against master as of 2.15-rc2.

Thanks,
Stefan

Re: [ANNOUNCE] Git for Windows 2.15.0

[Johannes Schindelin]

Hi team,

On Mon, 30 Oct 2017, Johannes Schindelin wrote:

> It is my pleasure to announce that Git for Windows 2.15.0 is available from:
> 
>   https://git-for-windows.github.io/

By the way, to everybody who tested the RC2: thank you so much. You caught
three bugs. Three bugs that did not enter Git for Windows v2.15.0.

Unfortunately, there is a bug in how I prepared those pre-releases, and
therefore you will be asked whether you want to "downgrade" to v2.15.0. I
hope to get to that bug well in time before the next round of release
candidates is due (or that a contributor beats me to it).

Ciao,
Johannes

Re: [PATCH v16 Part II 6/8] bisect--helper: `get_terms` & `bisect_terms` shell function in C

[Stephan Beyer]

On 10/27/2017 05:06 PM, Pranit Bauva wrote:
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 0f9c3e63821b8..ab0580ce0089a 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
[...]
> +static int bisect_terms(struct bisect_terms *terms, const char **argv, int 
> argc)
> +{
> + int i;
> +
> + if (get_terms(terms))
> + return error(_("no terms defined"));
> +
> + if (argc > 1)
> + return error(_("--bisect-term requires exactly one argument"));
> +
> + if (argc == 0)
> + return !printf(_("Your current terms are %s for the old state\n"
> +  "and %s for the new state.\n"),
> +  terms->term_good, terms->term_bad);

Same as in 1/8: you probably want "printf(...); return 0;" except there
is a good reason.

> +
> + for (i = 0; i < argc; i++) {
> + if (!strcmp(argv[i], "--term-good"))
> + printf(_("%s\n"), terms->term_good);
> + else if (!strcmp(argv[i], "--term-bad"))
> + printf(_("%s\n"), terms->term_bad);

The last two printfs: I think there is no point in translating "%s\n",
so using "%s\n" instead of _("%s\n") looks more reasonable.

> + else
> + error(_("BUG: invalid argument %s for 'git bisect 
> terms'.\n"
> +   "Supported options are: "
> +   "--term-good|--term-old and "
> +   "--term-bad|--term-new."), argv[i]);

Should this be "return error(...)"?

> + }
> +
> + return 0;
> +}
> +

Stephan

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

[tboegi]

From: Torsten Bögershausen 

Make it safer to normalize the line endings in a repository:
Files that had been commited with CRLF will be commited with LF.

The old way to normalize a repo was like this:
 # Make sure that there are not untracked files
 $ echo "* text=auto" >.gitattributes
 $ git read-tree --empty
 $ git add .
 $ git commit -m "Introduce end-of-line normalization"

The user must make sure that there are no untracked files,
otherwise they would have been added and tracked from now on.

The new "add ..renormalize" does not add untracked files:
 $ echo "* text=auto" >.gitattributes
 $ git add --renormalize .
 $ git commit -m "Introduce end-of-line normalization"

Note that "git add --renormalize " is the short form for
"git add -u --renormalize ".

While add it, document that the same renormalization may be needed,
whenever a clean filter is added or changed.

Helped-By: Junio C Hamano 
Signed-off-by: Torsten Bögershausen 
---

Second version:
- Removed the global flag
- Make clearer that the clean filters may need renormalization
- commit message improved

Documentation/git-add.txt   |  8 +++-
 Documentation/gitattributes.txt |  6 --
 builtin/add.c   | 28 ++--
 cache.h |  1 +
 read-cache.c| 30 +++---
 sha1_file.c | 16 ++--
 t/t0025-crlf-renormalize.sh | 30 ++
 7 files changed, 101 insertions(+), 18 deletions(-)
 create mode 100755 t/t0025-crlf-renormalize.sh

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index b700beaff5..09a08ce4c1 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 [verse]
 'git add' [--verbose | -v] [--dry-run | -n] [--force | -f] [--interactive | 
-i] [--patch | -p]
  [--edit | -e] [--[no-]all | --[no-]ignore-removal | [--update | -u]]
- [--intent-to-add | -N] [--refresh] [--ignore-errors] 
[--ignore-missing]
+ [--intent-to-add | -N] [--refresh] [--ignore-errors] 
[--ignore-missing] [--renormalize]
  [--chmod=(+|-)x] [--] [...]
 
 DESCRIPTION
@@ -175,6 +175,12 @@ for "git add --no-all ...", i.e. ignored removed 
files.
warning (e.g., if you are manually performing operations on
submodules).
 
+--renormalize::
+   Normalizes the line endings from CRLF to LF of tracked files.
+   This applies to files which are either "text" or "text=auto"
+   in .gitattributes (or core.autocrlf is true or input)
+   --renormalize implies -u
+
 --chmod=(+|-)x::
Override the executable bit of the added files.  The executable
bit is only changed in the index, the files on disk are left
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 4c68bc19d5..30687de81a 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -232,8 +232,7 @@ From a clean working directory:
 
 -
 $ echo "* text=auto" >.gitattributes
-$ git read-tree --empty   # Clean index, force re-scan of working directory
-$ git add .
+$ git add --renormalize .
 $ git status# Show files that will be normalized
 $ git commit -m "Introduce end-of-line normalization"
 -
@@ -328,6 +327,9 @@ You can declare that a filter turns a content that by 
itself is unusable
 into a usable content by setting the filter..required configuration
 variable to `true`.
 
+Note: Whenever the clean filter is changed, the repo should be renormalized:
+$ git add --renormalize .
+
 For example, in .gitattributes, you would assign the `filter`
 attribute for paths.
 
diff --git a/builtin/add.c b/builtin/add.c
index a648cf4c56..c42b50f857 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -26,6 +26,7 @@ static const char * const builtin_add_usage[] = {
 };
 static int patch_interactive, add_interactive, edit_interactive;
 static int take_worktree_changes;
+static int add_renormalize;
 
 struct update_callback_data {
int flags;
@@ -123,6 +124,25 @@ int add_files_to_cache(const char *prefix,
return !!data.add_errors;
 }
 
+static int renormalize_tracked_files(const struct pathspec *pathspec, int 
flags)
+{
+   int i, retval = 0;
+
+   for (i = 0; i < active_nr; i++) {
+   struct cache_entry *ce = active_cache[i];
+
+   if (ce_stage(ce))
+   continue; /* do not touch unmerged paths */
+   if (!S_ISREG(ce->ce_mode) && !S_ISLNK(ce->ce_mode))
+   continue; /* do not touch non blobs */
+   if (pathspec && !ce_path_match(ce, pathspec, NULL))
+   continue;
+   retval |= add_file_to_cache(ce->name, flags | HASH_RENORMALIZE);
+   }
+
+   return retval;
+}
+
 static char 

Re: [PATCH v2 3/4] Integrate hash algorithm support with repo setup

[Stefan Beller]

On Sat, Oct 28, 2017 at 11:12 AM, brian m. carlson
 wrote:
>
> Include repository.h in cache.h since we now need to have access to
> these struct and variable definitions.

Let's see how that works out. I remember having include issues
in the repository struct series.

>  };
>  extern const struct git_hash_algo hash_algos[GIT_HASH_NALGOS];
>
> +#define current_hash the_repository->hash_algo
> +
>  #if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)
>  #define DTYPE(de)  ((de)->d_type)
>  #else
> @@ -920,6 +923,7 @@ struct repository_format {
> int version;
> int precious_objects;
> int is_bare;
> +   int hash_algo;

I wonder if this (as well as the #defines in patch 2),
ought to be an enum { unknown-hash=0, sha1=1}.

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

[Stephan Beyer]

On 10/30/2017 02:38 PM, Stephan Beyer wrote:
> This last change is not necessary. You never use "res".

Whoops, ignore this!
I compared old and new diff and mixed something up, it seems.

Sorry,
Stephan

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

[Stefan Beller]

On Sat, Oct 28, 2017 at 11:12 AM, 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.

Cool.


> +
> +const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {
> +   {
> +   NULL,
> +   0x,
> +   0,
> +   0,
> +   0,
> +   NULL,
> +   NULL,
> +   NULL,
> +   NULL,
> +   NULL,

If we are fancy we could provide an appropriate die() call
as the function pointers. That way if you call these functions
by accident, you get a well worded warning instead of a segfault.

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

[Stefan Beller]

On Sat, Oct 28, 2017 at 11:12 AM, brian m. carlson
 wrote:
> We enumerate several different items as part of struct
> repository_format, but then actually set up those values using the
> global variables we've initialized from them.  Instead, let's pass a
> pointer to the structure down to the code where we enumerate these
> values, so we can later on use those values directly to perform setup.
>
> This technique makes it easier for us to determine additional items
> about the repository format (such as the hash algorithm) and then use
> them for setup later on, without needing to add additional global
> variables.  We can't avoid using the existing global variables since
> they're intricately intertwined with how things work at the moment, but
> this improves things for the future.

yup. looks good to me.

Thanks,
Stefan

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

[Stephan Beyer]

Also just a minor in addition to the others:

> @@ -153,9 +241,10 @@ int cmd_bisect__helper(int argc, const char **argv, 
> const char *prefix)
>   WRITE_TERMS,
>   BISECT_CLEAN_STATE,
>   CHECK_EXPECTED_REVS,
> - BISECT_RESET
> + BISECT_RESET,
> + BISECT_WRITE
>   } cmdmode = 0;
> - int no_checkout = 0;
> + int no_checkout = 0, res = 0;

This last change is not necessary. You never use "res".

Stephan

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

[Stephan Beyer]

Hi Pranit,

On 10/27/2017 05:06 PM, Pranit Bauva wrote:
> A big thanks to Stephan and Ramsay for patiently reviewing my series and
> helping me get this merged.

You're welcome. ;)

In addition to the things mentioned by the other reviewers, only a minor
thing:

> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 35d2105f941c6..12754448f7b6a 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
[...]> @@ -106,13 +112,48 @@ static void check_expected_revs(const char
**revs, int rev_nr)
>   }
>  }
>  
> +static int bisect_reset(const char *commit)
> +{
> + struct strbuf branch = STRBUF_INIT;
> +
> + if (!commit) {
> + if (strbuf_read_file(, git_path_bisect_start(), 0) < 1)
> + return !printf(_("We are not bisecting.\n"));

This is weird; I had to look up the return value of printf first before
I could understand what you are doing ;) I think that it is meant as a
shortcut for

printf(_("We are not bisecting.\n"));
return 0;

but please also express it with these two lines. (Or what is the point
of returning a non-zero value only in the case when nothing could be
printed?)

Best,
Stephan