Re: [PATCH] fast-import: checkpoint: dump branches/tags/marks even if object_count==0

[Eric Rannaud]

On Thu, Sep 28, 2017 at 8:51 PM, Junio C Hamano  wrote:
> I think that your patch the last round that feeds fd#8 in the
> foreground (i.e. fully trusting that the caller is sensibly giving
> input that produces no output) is already a good place to stop.
>
> Your patch this round that feeds fd#8 in the background, plus the
> attached patch (i.e. not trusting the caller as much and allowing it
> to use commands that outputs something, within reason), would also
> be a good place to stop.
>
> But I am not sure your patch this round alone is a good place to
> stop.  It somehow feels halfway either way.

I agree. If we're coding defensively against the caller, we do have to
include your patch to be effective, you're right. I reckon we likely
don't need to be quite so paranoid, at least until this has more
users.

Thanks.

Re: [PATCH] git-sh: Avoid sourcing scripts with git --exec-path

[Junio C Hamano]

Dridi Boukelmoune  writes:

> For end users making use of a custom exec path many commands will simply
> fail. Adding git's exec path to the PATH also allows overriding git-sh-*
> scripts, not just adding commands. One can then patch a script without
> tainting their system installation of git for example.

I think the first sentence is where you went wrong.  It seems that
you think this ought to work:

rm -fr $HOME/random-stuff
mkdir $HOME/random-stuff
echo "echo happy" >$HOME/random-stuff/git-happy
chmod +x $HOME/random-stuff/git-happy
GIT_EXEC_PATH=$HOME/random-stuff
export GIT_EXEC_PATH
# then...
git happy

But that is not the right/officially sanctioned/kosher way to add
custom git commands (adding your directory that has git-happy in it
to $PATH is).  GIT_EXEC_PATH is for the git-cmd binaries and scripts
we ship; it always is used to find non built-in commands, and even
for built-in commands, the command found via alias look-up is invoked
that way.

By insisting on overriding GIT_EXEC_PATH and not populating with
the stuff we ship, you'd need a workaround like your patch just to
make the scripts "work" again.  I have a feeling that even with your
patch you wouldn't be able to make non built-in commands, unless you
copy them (or write a thin wrapper that exec's the real thing).

So, instead of the two GIT_EXEC_PATH steps in the above example,
you can do

PATH=$HOME/random-stuff:$PATH

and you'll see "git happy" to work, I would think, without breaking
other things.

What's cooking in git.git (Sep 2017, #06; Fri, 29)

[Junio C Hamano]

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

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

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

--
[Graduated to "master"]

* hn/typofix (2017-09-22) 1 commit
  (merged to 'next' on 2017-09-25 at 489ad60f5b)
 + submodule.h: typofix


* ic/fix-filter-branch-to-handle-tag-without-tagger (2017-09-22) 4 commits
  (merged to 'next' on 2017-09-25 at c7550033df)
 + filter-branch: use hash-object instead of mktag
 + filter-branch: stash away ref map in a branch
 + filter-branch: preserve and restore $GIT_AUTHOR_* and $GIT_COMMITTER_*
 + filter-branch: reset $GIT_* before cleaning up

 "git filter-branch" cannot reproduce a history with a tag without
 the tagger field, which only ancient versions of Git allowed to be
 created.  This has been corrected.


* ik/userdiff-html-h-element-fix (2017-09-24) 1 commit
  (merged to 'next' on 2017-09-25 at e3cbe89672)
 + userdiff: fix HTML hunk header regexp

 The built-in pattern to detect the "function header" for HTML did
 not match .. elements without any attributes, which has
 been fixed.


* jc/merge-x-theirs-docfix (2017-09-25) 1 commit
  (merged to 'next' on 2017-09-26 at 5a7d954982)
 + merge-strategies: avoid implying that "-s theirs" exists

 The documentation for '-X' for merges was misleadingly
 written to suggest that "-s theirs" exists, which is not the case.


* jk/describe-omit-some-refs (2017-09-17) 1 commit
  (merged to 'next' on 2017-09-24 at c373c71279)
 + describe: fix matching to actually match all patterns
 (this branch is used by mk/describe-match-with-all.)

 "git describe --match" learned to take multiple patterns in v2.13
 series, but the feature ignored the patterns after the first one
 and did not work at all.  This has been fixed.


* jk/diff-blob (2017-09-22) 1 commit
  (merged to 'next' on 2017-09-25 at 38286c8ff5)
 + cat-file: handle NULL object_context.path

 "git cat-file --textconv" started segfaulting recently, which
 has been corrected.


* jk/doc-read-tree-table-asciidoctor-fix (2017-09-24) 1 commit
  (merged to 'next' on 2017-09-25 at 070163b964)
 + doc: put literal block delimiter around table

 A docfix.


* jk/fallthrough (2017-09-22) 3 commits
  (merged to 'next' on 2017-09-25 at ad96c37620)
 + consistently use "fallthrough" comments in switches
 + curl_trace(): eliminate switch fallthrough
 + test-line-buffer: simplify command parsing

 Many codepaths have been updated to squelch -Wimplicit-fallthrough
 warnings from Gcc 7 (which is a good code hygiene).


* jm/status-ignored-directory-optim (2017-09-19) 1 commit
  (merged to 'next' on 2017-09-24 at ca50f5ed41)
 + Improve performance of git status --ignored

 "git status --ignored", when noticing that a directory without any
 tracked path is ignored, still enumerated all the ignored paths in
 the directory, which is unnecessary.  The codepath has been
 optimized to avoid this overhead.


* js/win32-lazyload-dll (2017-09-26) 1 commit
  (merged to 'next' on 2017-09-26 at 04577bf1c5)
 + Win32: simplify loading of DLL functions

 Add a helper in anticipation for its need in a future topic RSN.


* jt/fast-export-copy-modify-fix (2017-09-21) 1 commit
  (merged to 'next' on 2017-09-24 at c02bfe1902)
 + fast-export: do not copy from modified file

 "git fast-export" with -M/-C option issued "copy" instruction on a
 path that is simultaneously modified, which was incorrect.


* ks/doc-use-camelcase-for-config-name (2017-09-25) 1 commit
  (merged to 'next' on 2017-09-26 at 7b4d2115af)
 + doc: camelCase the config variables to improve readability

 Doc update.


* ma/leakplugs (2017-09-24) 6 commits
  (merged to 'next' on 2017-09-25 at 69d381a96a)
 + pack-bitmap[-write]: use `object_array_clear()`, don't leak
 + object_array: add and use `object_array_pop()`
 + object_array: use `object_array_clear()`, not `free()`
 + leak_pending: use `object_array_clear()`, not `free()`
 + commit: fix memory leak in `reduce_heads()`
 + builtin/commit: fix memory leak in `prepare_index()`

 Memory leaks in various codepaths have been plugged.


* mk/describe-match-with-all (2017-09-20) 2 commits
  (merged to 'next' on 2017-09-24 at f96d58dd83)
 + describe: teach --match to handle branches and remotes
 + Merge branch 'jk/describe-omit-some-refs' into mk/describe-match-with-all

 "git describe --match " has been taught to play well with
 the "--all" option.


* mk/diff-delta-avoid-large-offset (2017-08-11) 1 commit
  (merged to 'next' on 2017-09-26 at b4983ac027)
 + diff-delta: do not allow delta offset truncation

 The delta format used in the packfile cannot reference data at
 offset larger than what can be expressed in 4-byte, but 

Re: [PATCH] gitk: expand $config_file_tmp before reporting to user

[Max Kirillov]

On Thu, Sep 28, 2017 at 02:31:17PM +0200, Johannes Schindelin wrote:
>>> Max Kirillov  writes:
 Tilda-based path may confise some users. First, tilda is not known
 for Window users, second, it may point to unexpected location
 depending on various environment setup.

 Expand the path to "nativename", so that ~/.config/git/gitk-tmp
 would be "C:\Users\user\.config\git\gitk-tmp", for example.
 It should be less cryptic

> Thanks, Max, for your contribution!

I do what I can. Just noticed s question at SO about it
(https://stackoverflow.com/questions/46450479/how-to-remove-the-stale-gitk-tmp-file)
Provided that I was author of the message, it looked like
something for me to fix.

> Sound good?

As Junio noticed, it would be more reliable to show full
path, and error message does not have to be very nice
anyway. Also, gitk is already too big and I always feel bad
when adding stuff to it, so let's save couple of lines by
not adding another "if".

-- 
Max

Re: [PATCH] git-sh: Avoid sourcing scripts with git --exec-path

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder  writes:

>> This has been broken for a while, but better late than never to
>> address it.
>
> I am not sure if this is broken in the first place.  We do want to
> make sure that the scripted porcelains will source the shell helper
> library from matching Git release.  The proposed patch goes directly
> against that and I do not see how it could be an improvement.

It used to be that git allowed setting a colon-separated list of paths
in GIT_EXEC_PATH.  (Very long ago, I relied on that feature.)  If we
can restore that functionality without too much cost, then I think
it's worthwhile.

But the cost in this patch for that is pretty high.  And

$ git log -S'$(git --exec-path)/'

tells me that colon-separated paths in GIT_EXEC_PATH did not work for
some use cases as far back as 2006.  Since 2008 the documentation has
encouraged using "git --exec-path" in a way that does not work with
colon-separated paths, too.  I hadn't realized it was so long.  Given
that it hasn't been supported for more than ten years, I was wrong to
read this as a bug report --- instead, it's a feature request.

In that context, this cost is likely not worth paying.

I wonder if there's another way to achieve this patch's goal.  E.g.
what if there were a way to specify some paths git could search for
custom commands, separate from "git --exec-path"?

Putting the custom commands on the $PATH seems nicer unless you're
trying to override the implementation of an existing git command.
And we already discourage overriding the implementation of an existing
git command (as it's open source, you can patch them instead), so...

Another possible motivation (the one that led me to use this mechnism
long ago) is avoiding providing the dashed form git-$cmd in $PATH.  I
think time has shown that having git-$cmd in $PATH is not too painful.

Thanks and hope that helps,
Jonathan

Re: [PATCH] doc: correct command formatting

[Junio C Hamano]

Jonathan Nieder  writes:

> Separate from this example: yes, I think adopting Linux's Reviewed-by
> convention would be a good thing.  When I see a positive reply to a
> patch, I often wonder whether an ack or a fuller reviewed-by is
> intended, and Linux's way of formalizing that appeals to me.
>
> I'll try sending a patch to add it to SubmittingPatches tomorrow
> morning (Stefan had also been hinting recently about this being
> something worth trying).

Thanks.  

I agree with the goal of educating list participants not to throw
Reviewed-by: without reading the patches too carelessly.  As akpm
once said in <20121003143200.69a50aad.a...@linux-foundation.org>,
"Looks ok to me from a quick look" is not a review.

> > No, lib/lzo has no identifiable maintainer.  I suggest you proceed as
> > follows:
> > 
> > - Post the entire patch series to lkml for review (I'd like a cc please)
> 
> Already happened, multiple people reviewed and tested.

um, I would not consider "Looks ok to me from a quick look." and "I
couldn't tell from the github view, but I assume you follow standard
coding style." to indicate a rigorous code review!

That's the problem with the git presentation: hardly anyone reads the
patches and there is no patch for a reviewer to reply to.

So please send the patches out for review.  One at a time, via email.

Re: [PATCH] git-sh: Avoid sourcing scripts with git --exec-path

[Junio C Hamano]

Jonathan Nieder  writes:

> This has been broken for a while, but better late than never to
> address it.

I am not sure if this is broken in the first place.  We do want to
make sure that the scripted porcelains will source the shell helper
library from matching Git release.  The proposed patch goes directly
against that and I do not see how it could be an improvement.

>> --- a/contrib/rerere-train.sh
>> +++ b/contrib/rerere-train.sh
>> @@ -7,7 +7,8 @@ USAGE="$me rev-list-args"
>>  
>>  SUBDIRECTORY_OK=Yes
>>  OPTIONS_SPEC=
>> -. "$(git --exec-path)/git-sh-setup"
>> +PATH="$(git --exec-path):$PATH"
>> +. git-sh-setup
>
> This makes me similarly unhappy about PATH pollution, but it may be
> that there's nothing to be done about that.

To me, all the instances of the unhappiness in your review seem to
be caused by the unjustified declaration that it is bad to source
from the directory "gti --exec-path" reports.  If a user wants to
tweak things Git does, why should the user futz with his own copy
of sh-setup and force scripted Porcelains to read from it, which
would only affect the scripted Porcelains and have no chance of
affecting compiled commands?  Is building from the source so bad for
an open source tool?

Re: [PATCH] fast-import: checkpoint: dump branches/tags/marks even if object_count==0

[Junio C Hamano]

"Eric Rannaud"  writes:

> Junio, this last version addresses your last remark regarding the
> potential for the cat $input_file sequence to block when the FIFOs are
> unbuffered.
>
> The concern is mainly theoretical (*if* the shell function is used
> correctly): fast-import will not start writing to its own output until
> it has fully consumed its input (as the only command generating output
> should be the last one). Nevertheless, in this version the write is done
> in the background.

I agree that the concern is theoretical.  Without this fix, we may
not be able to feed the input fully up to the final 'progress
checkpoint' command---because the other side is not reading, we may
get stuck while attempting to write "checkpoint" or "progress
checkpoint", and may never get to read what fast-import says to get
us unstuck.

But if we wanted to solve the theoretical issue (i.e. the command
sequence the user of this helper shell function gives us _might_
trigger an output from fast-import) fully, I do not think
backgrounding the feeding side is sufficient.  The code that reads
fd#9 would need to become a while loop that reads and discards extra
output until we see the "progress checkpoint", at least, perhaps
like the attached patch.

But even with such a fix, we are still at the mercy of the caller of
the helper---the helper will get broken if the input happened to
have a "progress checkpoint" command itself.  There has to be a
"good enough" place to stop.

I think that your patch the last round that feeds fd#8 in the
foreground (i.e. fully trusting that the caller is sensibly giving
input that produces no output) is already a good place to stop.

Your patch this round that feeds fd#8 in the background, plus the
attached patch (i.e. not trusting the caller as much and allowing it
to use commands that outputs something, within reason), would also
be a good place to stop.

But I am not sure your patch this round alone is a good place to
stop.  It somehow feels halfway either way.

 t/t9300-fast-import.sh | 30 --
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 74ba70874b..d47560b634 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -3159,18 +3159,17 @@ background_import_then_checkpoint () {
echo "progress checkpoint"
) >&8 &
 
-   error=0
-   if read output <&9
-   then
-   if ! test "$output" = "progress checkpoint"
+   error=1 ;# assume the worst
+   while read output <&9
+   do
+   if test "$output" = "progress checkpoint"
then
-   echo >&2 "no progress checkpoint received: $output"
-   error=1
+   error=0
+   break
fi
-   else
-   echo >&2 "failed to read fast-import output"
-   error=1
-   fi
+   # otherwise ignore cruft
+   echo >&2 "cruft: $output"
+   done
 
if test $error -eq 1
then
@@ -3186,6 +3185,17 @@ background_import_still_running () {
fi
 }
 
+test_expect_success PIPE 'V: checkpoint helper does not get stuck with extra 
output' '
+   cat >input <<-INPUT_END &&
+   progress foo
+   progress bar
+
+   INPUT_END
+
+   background_import_then_checkpoint "" input &&
+   background_import_still_running
+'
+
 test_expect_success PIPE 'V: checkpoint updates refs after reset' '
cat >input <<-\INPUT_END &&
reset refs/heads/V
-- 
2.14.2-820-gd7428e091c

Re: [PATCH] git-sh: Avoid sourcing scripts with git --exec-path

[Jonathan Nieder]

Hi,

Dridi Boukelmoune wrote:

> For end users making use of a custom exec path many commands will simply
> fail. Adding git's exec path to the PATH also allows overriding git-sh-*
> scripts, not just adding commands. One can then patch a script without
> tainting their system installation of git for example.
>
> Signed-off-by: Dridi Boukelmoune 

This has been broken for a while, but better late than never to
address it.

[...]
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -151,6 +151,28 @@ For shell scripts specifically (not exhaustive):
> interface translatable. See "Marking strings for translation" in
> po/README.
>  
> + - When sourcing a "git-sh-*" file using "git --exec-path" make sure
> +   to only use its base name.
> +
> + (incorrect)
> + . "$(git --exec-path)/git-sh-setup"
> +
> + (correct)
> + . git-sh-setup

I wonder if we can make this so intuitive that it doesn't need
mentioning in CodingGuidelines.  What if the test harness
t/test-lib.sh were to set a GIT_EXEC_PATH with multiple directories in
it?  That way, authors of new commands would not have to be careful to
look out for this issue proactively because the testsuite would catch
it.

[...]
> +++ b/contrib/convert-grafts-to-replace-refs.sh
> @@ -5,7 +5,8 @@
>  
>  GRAFTS_FILE="${GIT_DIR:-.git}/info/grafts"
>  
> -. $(git --exec-path)/git-sh-setup
> +PATH="$(git --exec-path):$PATH"
> +. git-sh-setup

I wish there were a simple way to do this that doesn't involve
polluting $PATH with the dashed commands.  E.g. we can do something
like

OIFS=$IFS
IFS=:
set -f
for d in $(git --exec-path)
do
if test -e "$d/git-sh-setup"
then
. "$d/git-sh-setup"
break
fi
done
set +f
IFS=$OIFS

but that is not very simple.  Something like

old_PATH=$PATH
PATH=$(git --exec-path):$PATH
. git-sh-setup
PATH=$old_PATH

looks like it could work, but it would undo the work git-sh-setup
does to set a sane $PATH on platforms like Solaris.

> --- a/contrib/rerere-train.sh
> +++ b/contrib/rerere-train.sh
> @@ -7,7 +7,8 @@ USAGE="$me rev-list-args"
>  
>  SUBDIRECTORY_OK=Yes
>  OPTIONS_SPEC=
> -. "$(git --exec-path)/git-sh-setup"
> +PATH="$(git --exec-path):$PATH"
> +. git-sh-setup

This makes me similarly unhappy about PATH pollution, but it may be
that there's nothing to be done about that.

[...]
> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -32,7 +32,6 @@ squashmerge subtree changes as a single commit
>  "
>  eval "$(echo "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit 
> $?)"
>  
> -PATH=$PATH:$(git --exec-path)
>  . git-sh-setup

This looks like a change that could be separated into a separate
patch, both because it is to contrib/subtree (which is maintained
separately) and because it is not necessary for the goal described in
this patch's commit message.  I do like this change, since it improves
consistency with other commands.

> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -44,7 +44,7 @@ git_broken_path_fix () {
>  # @@BROKEN_PATH_FIX@@
>  
>  # Source git-sh-i18n for gettext support.
> -. "$(git --exec-path)/git-sh-i18n"
> +. git-sh-i18n

Do git-mergetool--lib.txt, git-parse-remote.txt, git-sh-i18n.txt,
and git-sh-setup.txt in Documentation/ need the same treatment?

Summary: I like the goal of this patch but I am nervous about the
side-effect it introduces of PATH pollution.  Is there a way around
that?  If there isn't, then this needs a few tweaks and then it should
be ready to go.

Thanks and hope that helps,
Jonathan

Re: [PATCH] doc: correct command formatting

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder  writes:
>> Andreas Heiduk wrote:

>>> +1, Thanks for spotting.
>>
>> Thanks for looking it over.  Can we add your Reviewed-by?  (See [1]
>> for what this means.)
>
> I would just do "Acked-by: Andreas" after seeing such an obvious
> admission of guilt & appreciation for fixing in the exchange.

Oh!  I had missed that context.

Your instinct would have been right (and is born out by Andreas's
reply to me).

I was just fishing, but in this context there was no reason to fish
for more than the Ack that was already there.

> Would we rather want to make it more formal like how Linux folks do
> the Reviewed-by: thing?

Separate from this example: yes, I think adopting Linux's Reviewed-by
convention would be a good thing.  When I see a positive reply to a
patch, I often wonder whether an ack or a fuller reviewed-by is
intended, and Linux's way of formalizing that appeals to me.

I'll try sending a patch to add it to SubmittingPatches tomorrow
morning (Stefan had also been hinting recently about this being
something worth trying).

Thank you,
Jonathan

[PATCH] fast-import: checkpoint: dump branches/tags/marks even if object_count==0

[Eric Rannaud]

The checkpoint command cycles packfiles if object_count != 0, a sensible
test or there would be no pack files to write. Since 820b931012, the
command also dumps branches, tags and marks, but still conditionally.
However, it is possible for a command stream to modify refs or create
marks without creating any new objects.

For example, reset a branch (and keep fast-import running):

$ git fast-import
reset refs/heads/master
from refs/heads/master^

checkpoint

but refs/heads/master remains unchanged.

Other example: a commit command that re-creates an object that already
exists in the object database.

The man page also states that checkpoint "updates the refs" and that
"placing a progress command immediately after a checkpoint will inform
the reader when the checkpoint has been completed and it can safely
access the refs that fast-import updated". This wasn't always true
without this patch.

This fix unconditionally calls dump_{branches,tags,marks}() for all
checkpoint commands. dump_branches() and dump_tags() are cheap to call
in the case of a no-op.

Add tests to t9300 that observe the (non-packfiles) effects of
checkpoint.

Signed-off-by: Eric Rannaud 
---
 fast-import.c  |   6 +--
 t/t9300-fast-import.sh | 130 +
 2 files changed, 133 insertions(+), 3 deletions(-)


Junio, this last version addresses your last remark regarding the
potential for the cat $input_file sequence to block when the FIFOs are
unbuffered.

The concern is mainly theoretical (*if* the shell function is used
correctly): fast-import will not start writing to its own output until
it has fully consumed its input (as the only command generating output
should be the last one). Nevertheless, in this version the write is done
in the background.

Thanks!


diff --git a/fast-import.c b/fast-import.c
index 35bf671f12c4..d5e4cf0bad41 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -3189,10 +3189,10 @@ static void checkpoint(void)
checkpoint_requested = 0;
if (object_count) {
cycle_packfile();
-   dump_branches();
-   dump_tags();
-   dump_marks();
}
+   dump_branches();
+   dump_tags();
+   dump_marks();
 }
 
 static void parse_checkpoint(void)
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 67b8c50a5ab4..8f583e8a22c1 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -3120,4 +3120,134 @@ test_expect_success 'U: validate root delete result' '
compare_diff_raw expect actual
 '
 
+###
+### series V (checkpoint)
+###
+
+# The commands in input_file should not produce any output on the file
+# descriptor set with --cat-blob-fd (or stdout if unspecified).
+#
+# To make sure you're observing the side effects of checkpoint *before*
+# fast-import terminates (and thus writes out its state), check that the
+# fast-import process is still running using background_import_still_running
+# *after* evaluating the test conditions.
+background_import_then_checkpoint () {
+   options=$1
+   input_file=$2
+
+   mkfifo V.input
+   exec 8<>V.input
+   rm V.input
+
+   mkfifo V.output
+   exec 9<>V.output
+   rm V.output
+
+   git fast-import $options <&8 >&9 &
+   echo $! >V.pid
+   # We don't mind if fast-import has already died by the time the test
+   # ends.
+   test_when_finished "exec 8>&-; exec 9>&-; kill $(cat V.pid) || true"
+
+   # Start in the background to ensure we adhere strictly to (blocking)
+   # pipes writing sequence. We want to assume that the write below could
+   # block, e.g. if fast-import blocks writing its own output to &9
+   # because there is no reader on &9 yet.
+   ( cat "$input_file"
+   echo "checkpoint"
+   echo "progress checkpoint" ) >&8 &
+
+   error=0
+   if read output <&9
+   then
+   if ! test "$output" = "progress checkpoint"
+   then
+   echo >&2 "no progress checkpoint received: $output"
+   error=1
+   fi
+   else
+   echo >&2 "failed to read fast-import output"
+   error=1
+   fi
+
+   if test $error -eq 1
+   then
+   false
+   fi
+}
+
+background_import_still_running () {
+   if ! kill -0 "$(cat V.pid)"
+   then
+   echo >&2 "background fast-import terminated too early"
+   false
+   fi
+}
+
+test_expect_success PIPE 'V: checkpoint updates refs after reset' '
+   cat >input <<-\INPUT_END &&
+   reset refs/heads/V
+   from refs/heads/U
+
+   INPUT_END
+
+   background_import_then_checkpoint "" input &&
+   test "$(git rev-parse --verify V)" = "$(git rev-parse --verify U)" &&
+   background_import_still_running
+'
+
+test_expect_success PIPE 'V: checkpoint updates refs and marks after commit' '

Re: [PATCH 1/1] fast-import: checkpoint: dump branches/tags/marks even if object_count==0

[Junio C Hamano]

Adam Dinwoodie  writes:

>> Also adding the necessary PIPE prereq, as pointed out by Ramsay Jones.
>
> Cygwin doesn't have the PIPE prereq; I've just confirmed that the
> previous version of this patch has t9300 failing on Cygwin, but this
> version passes.

Thanks for a report.  So the patch should be good to go, it seems to
me.

Eric, thanks again for working on this one.

Re: [PATCH v8 00/12] Fast git status via a file system watcher

[Junio C Hamano]

Ben Peart  writes:

> The only behavioral change from V7 is the removal of unnecessary uses of
> CE_MATCH_IGNORE_FSMONITOR.  With a better understanding of *why* the
> CE_MATCH_IGNORE_* flags are used, it is now clear they are not required
> in most cases where CE_MATCH_IGNORE_FSMONITOR was being passed out of an
> abundance of caution.

The reviews and updates after this round was posted were to

 * 01/12 had an obvious pointer-vs-pointee thinko, which I think I
   have locally fixed;

 * 08/12 forgot to add a new test executable to .gitignore file,
   which I think I have locally fixed, too.

Any other review comments and suggestions for improvements?
Otherwise I am tempted to declare victory and merge this to 'next'
soonish.

For reference, here is the interdiff between what was posted as v8
and what I have on 'pu'.

Thanks.

 compat/bswap.h  | 4 ++--
 t/helper/.gitignore | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git b/compat/bswap.h a/compat/bswap.h
index 6b22c46214..5078ce5ecc 100644
--- b/compat/bswap.h
+++ a/compat/bswap.h
@@ -183,8 +183,8 @@ static inline uint32_t get_be32(const void *ptr)
 static inline uint64_t get_be64(const void *ptr)
 {
const unsigned char *p = ptr;
-   return  (uint64_t)get_be32(p[0]) << 32 |
-   (uint64_t)get_be32(p[4]) <<  0;
+   return  (uint64_t)get_be32([0]) << 32 |
+   (uint64_t)get_be32([4]) <<  0;
 }
 
 static inline void put_be32(void *ptr, uint32_t value)
diff --git b/t/helper/.gitignore a/t/helper/.gitignore
index f9328eebdd..87a648a7cf 100644
--- b/t/helper/.gitignore
+++ a/t/helper/.gitignore
@@ -5,6 +5,7 @@
 /test-delta
 /test-drop-caches
 /test-dump-cache-tree
+/test-dump-fsmonitor
 /test-dump-split-index
 /test-dump-untracked-cache
 /test-fake-ssh

Re: [PATCH v3 00/21] Read `packed-refs` using mmap()

[Junio C Hamano]

Jeff King  writes:

> On Mon, Sep 25, 2017 at 09:59:57AM +0200, Michael Haggerty wrote:
>
>> This is v3 of a patch series that changes the reading and caching of
>> the `packed-refs` file to use `mmap()`. Thanks to Stefan, Peff, Dscho,
>> and Junio for their comments about v2. I think I have addressed all of
>> the feedback from v1 [1] and v2 [2].
>> 
>> This version has only minor changes relative to v2:
>> 
>> * Fixed a trivial error in the commit message for patch 08.
>> 
>> * In patch 13:
>> 
>>   * In the commit message, explain the appearance of `MMAP_TEMPORARY`
>> even though it is not yet treated differently than `MMAP_NONE`.
>> 
>>   * In `Makefile`, don't make `USE_WIN32_MMAP` imply
>> `MMAP_PREVENTS_DELETE`.
>> 
>>   * Correct the type of a local variable from `size_t` to `ssize_t`.
>
> Thanks, this version addresses all my nits.

Dscho's  "does
not seem to break windows" was against the previous round, but it
seems that https://travis-ci.org/git/git/jobs/280305212 passed the
iteration of 'pu' at 044a672 which contained this version, so let's
merge this down to 'next'.

Thanks.

Re: RFC: Design and code of partial clones (now, missing commits and trees OK)

[Junio C Hamano]

Jonathan Tan  writes:

> I've pushed a new version:
>
> https://github.com/jonathantanmy/git/tree/partialclone3

Just FYI, the reason why I commented only on the first patch in your
previous series at GitHub wasn't because I found the others perfect
and nothing to comment on.  It was because I found it extremely
painful to conduct review and comment in the webform and gave up
while trying to review the series that way just after doing a single
patch.

I also found it frustrating that it is not even obvious which one of
the many patches in the series have been already commented on,
without clicking to each and every commit (and back), even when the
result is to find that nobody has commented on them yet.

Re: [PATCH] doc: correct command formatting

[Junio C Hamano]

Jonathan Nieder  writes:

> Andreas Heiduk wrote:
>
>> +1, Thanks for spotting.
>
> Thanks for looking it over.  Can we add your Reviewed-by?  (See [1]
> for what this means.)

I would just do "Acked-by: Andreas" after seeing such an obvious
admission of guilt & appreciation for fixing in the exchange.  

Would we rather want to make it more formal like how Linux folks do
the Reviewed-by: thing?

Re: RFC: Design and code of partial clones (now, missing commits and trees OK)

[Jonathan Tan]

On Fri, 15 Sep 2017 13:43:43 -0700
Jonathan Tan  wrote:

> For those interested in partial clones and/or missing objects in repos,
> I've updated my original partialclone patches to not require an explicit
> list of promises. Fetch/clone still only permits exclusion of blobs, but
> the infrastructure is there for a local repo to support missing trees
> and commits as well.
> 
> They can be found here:
> 
> https://github.com/jonathantanmy/git/tree/partialclone2

I've pushed a new version:

https://github.com/jonathantanmy/git/tree/partialclone3

Besides some small changes as requested by comments on the GitHub
repository, I've also updated the code to do the following:
 - clarified terminology - in particular, I've tried to avoid
   "promised", only using "promisor object" to denote objects that the
   local repo knows that the promisor remote has, whether the local repo
   has it or not
 - restored bulk checkout functionality (so now you can clone with
   --blob-max-bytes=0)
 - a fix to fetch-pack to restore a global flag after it uses it, so
   commands like "git log -S" still work (but to test this, I used
   --blob-max-bytes=20 with the Git repository, because batch
   fetching is not implemented for commands like these)

In its current form, the code is already useful for situations like:
 - a large repository with many blobs in which the client only needs to
   checkout, at most, and does not need to search through history
   locally, and
 - a repository with a few large blobs, where the client still can
   search through history as long as the client is online

Re: [Bug/Solution] Git hangs in compat/poll/poll.c on HPE NonStop

[Junio C Hamano]

[jch: a patch that hopefully is applicable is attached at the end;
 I'd appreciate input from Paolo, the contributor of the original
 upstream]

"Randall S. Becker"  writes:

> After a whole lot of investigating, we (it is a large "we") have discovered
> the reason for the hang we occasionally get in git-upload-pack on HPE
> NonStop servers - reported here well over a year ago. This resulted from a
> subtle check that the operating system does on file descriptors. When it
> sees random values in pfd[i].revents,...

What do you mean by "random values"?  Where do these random values
come from?  The original code the patch touches is _not_ setting
anything, so it is leaving it uninitialized and that is seen by the
system?  If that is the case perhaps should we clear it before we
call into this codepath?

>  There is a non-destructive fix
> that I would like to propose for this that I have already tested.

I am not sure in what sense this is "non-destructive"; we left the
value as it was when we didn't notice anything happening in
compute_revents().  Now we explicitly destroy the value there was
(just like we do in the case where the corresponding file descriptor
was negative).  Maybe overwriting with 0 is the right thing, but I
wouldn't call it "non-destructive", though.  Puzzling...

> --- a/compat/poll/poll.c
> +++ b/compat/poll/poll.c
> @@ -438,6 +438,10 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
> pfd[i].revents = happened;
> rc++;
>   }
> +else
> +  {
> +pfd[i].revents = 0;
> +  }
>}
>
>return rc;

FYI, the upstream gnulib rewrites this part of the code with
d42461c3 ("poll: fixes for large fds", 2015-02-20) quite a bit, and
it reads like this:

+{
+  pfd[i].revents = (pfd[i].fd < 0
+? 0
+: compute_revents (pfd[i].fd, pfd[i].events,
+   , , ));
+  rc += pfd[i].revents != 0;
+}

The code before your fix (and before d42461c3) used to assign to
pfd[i].revents only when compute_revents() gave us non-zero value,
but with d42461c3 in the upstream, it pfd[i].revents is assigned
the return value from compute_revents() even if the value returned
is 0.  And rc is incremented only when that value is non-zero.

The result of applying your patch is equivalent, so after all, I
tend to think that your patch is the right fix in the context of the
code we have (i.e. the older code we borrowed from them).

I wonder if we want to refresh the borrowed copy of poll.c to a
newer snapshot someday, but that is totally a separate topic.  At
least, I now know that your fix in this patch will not be broken due
to d42461c3 updating the code in a slightly different way ;-)

Randall, I forged your Sign-off in the patch below, but please say
it is OK, after reading Documentation/SubmittingPatches.

Thanks.

-- >8 --
From: Randall S. Becker 
Subject: poll.c: always set revents, even if to zero.

Match what happens to pfd[i].revents when compute_revents() returns
0 to the upstream gnulib's commit d42461c3 ("poll: fixes for large
fds", 2015-02-20).  The revents field is set to 0, without
incrementing the value rc to be returned from the function.

This fixes occasional hangs in git-upload-pack on NPE NonStop.

Signed-off-by: Randall S. Becker 
Signed-off-by: Junio C Hamano 
---
 compat/poll/poll.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/compat/poll/poll.c b/compat/poll/poll.c
index b10adc780f..ae03b74a6f 100644
--- a/compat/poll/poll.c
+++ b/compat/poll/poll.c
@@ -438,6 +438,10 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
pfd[i].revents = happened;
rc++;
  }
+   else
+ {
+   pfd[i].revents = 0;
+ }
   }
 
   return rc;

Re: [PATCH Outreachy] mru: use double-linked list from list.h

[Jeff King]

On Thu, Sep 28, 2017 at 08:38:39AM +, Olga Telezhnaya wrote:

> diff --git a/packfile.c b/packfile.c
> index f69a5c8d607af..ae3b0b2e9c09a 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -876,6 +876,7 @@ void prepare_packed_git(void)
>   for (alt = alt_odb_list; alt; alt = alt->next)
>   prepare_packed_git_one(alt->path, 0);
>   rearrange_packed_git();
> + INIT_LIST_HEAD(_git_mru.list);
>   prepare_packed_git_mru();
>   prepare_packed_git_run_once = 1;
>  }

I was thinking on this hunk a bit more, and I think it's not quite
right.

The prepare_packed_git_mru() function will clear the mru list and then
re-add each item from the packed_git list. But by calling
INIT_LIST_HEAD() here, we're effectively clearing the packed_git_mru
list, and we end up leaking whatever was on the list before.

So for the first call to prepare_packed_git, we really need this
INIT_LIST_HEAD() call. But for subsequent calls (which come from
reprepare_packed_git()), we must not call it.

There are a few ways to work around it that I can think of:

  1. Check whether packed_git_mru.list.head is NULL, and only initialize
 in that case.

  2. Use a static initializer for packed_git_mru.list, so that we don't
 have do the first-time initializing here.

  3. Teach reprepare_packed_git() to do the mru_clear() call, so that we
 know the list is empty when we get here.

One final and more invasive option is to stop regenerating the
packed_git_mru list from scratch during each prepare_packed_git(). I did
it that way so that we start with the same order that
rearrange_packed_git() will give us, but I'm not sure how much value
that has in practice (it probably had a lot more when we didn't have the
mru, and the time-sorted pack order helped find recent objects more
quickly).

The alternative would be to just teach install_packed_git() to add each
newly-added pack to the mru list, and then never clear the list (and we
wouldn't need an mru_clear() at all, then).

-Peff

[PATCH] git-sh: Avoid sourcing scripts with git --exec-path

[Dridi Boukelmoune]

For end users making use of a custom exec path many commands will simply
fail. Adding git's exec path to the PATH also allows overriding git-sh-*
scripts, not just adding commands. One can then patch a script without
tainting their system installation of git for example.

Signed-off-by: Dridi Boukelmoune 
---

Hi,

I've been bothered by this problem ever since I upgraded my system to
Fedora 26, and I grew tired of locally patching git-sh-setup after every
git-core update. So I decided to look at the instructions on how to send
patches to the Git project, and here is my first patch.

I hope you will find it appropriate, I didn't study the test suite to
have it enforce that files shouldn't be sourced from the "system"
installation. I couldn't reproduce it after a quick look, and I'm not
familiar enough to tinker with it yet, so I reached my trial quota
before I could figure things out.

Best Regards,
Dridi

 Documentation/CodingGuidelines| 22 ++
 contrib/convert-grafts-to-replace-refs.sh |  3 ++-
 contrib/rerere-train.sh   |  3 ++-
 contrib/subtree/git-subtree.sh|  1 -
 git-sh-setup.sh   |  2 +-
 5 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index c4cb5ff0d..fdc0d3318 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -151,6 +151,28 @@ For shell scripts specifically (not exhaustive):
interface translatable. See "Marking strings for translation" in
po/README.
 
+ - When sourcing a "git-sh-*" file using "git --exec-path" make sure
+   to only use its base name.
+
+   (incorrect)
+   . "$(git --exec-path)/git-sh-setup"
+
+   (correct)
+   . git-sh-setup
+
+   Otherwise for a user with a custom "GIT_EXEC_PATH=/foo" the shell
+   expects "/foo:/usr/libexec/git-core/git-sh-setup" or something
+   similar depending on the installation. The git command already
+   adds the git exec path to the regular path before executing a
+   command.
+
+   For scripts that aren't run via the git command, add the git exec
+   path to the path instead.
+
+   (correct)
+   PATH="$(git --exec-path):$PATH"
+   . git-sh-setup
+
  - We do not write our "test" command with "-a" and "-o" and use "&&"
or "||" to concatenate multiple "test" commands instead, because
the use of "-a/-o" is often error-prone.  E.g.
diff --git a/contrib/convert-grafts-to-replace-refs.sh 
b/contrib/convert-grafts-to-replace-refs.sh
index 0cbc917b8..f7d2fab03 100755
--- a/contrib/convert-grafts-to-replace-refs.sh
+++ b/contrib/convert-grafts-to-replace-refs.sh
@@ -5,7 +5,8 @@
 
 GRAFTS_FILE="${GIT_DIR:-.git}/info/grafts"
 
-. $(git --exec-path)/git-sh-setup
+PATH="$(git --exec-path):$PATH"
+. git-sh-setup
 
 test -f "$GRAFTS_FILE" || die "Could not find graft file: '$GRAFTS_FILE'"
 
diff --git a/contrib/rerere-train.sh b/contrib/rerere-train.sh
index 52ad9e41f..07bad67e6 100755
--- a/contrib/rerere-train.sh
+++ b/contrib/rerere-train.sh
@@ -7,7 +7,8 @@ USAGE="$me rev-list-args"
 
 SUBDIRECTORY_OK=Yes
 OPTIONS_SPEC=
-. "$(git --exec-path)/git-sh-setup"
+PATH="$(git --exec-path):$PATH"
+. git-sh-setup
 require_work_tree
 cd_to_toplevel
 
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index dec085a23..1d61f75d0 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -32,7 +32,6 @@ squashmerge subtree changes as a single commit
 "
 eval "$(echo "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit $?)"
 
-PATH=$PATH:$(git --exec-path)
 . git-sh-setup
 
 require_work_tree
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 378928518..12e1220f8 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -44,7 +44,7 @@ git_broken_path_fix () {
 # @@BROKEN_PATH_FIX@@
 
 # Source git-sh-i18n for gettext support.
-. "$(git --exec-path)/git-sh-i18n"
+. git-sh-i18n
 
 die () {
die_with_status 1 "$@"
-- 
2.13.5

Re: [PATCH] doc: correct command formatting

[Andreas Heiduk]

Am 28.09.2017 um 21:34 schrieb Jonathan Nieder:
> Andreas Heiduk wrote:
> 
>> +1, Thanks for spotting.
> 
> Thanks for looking it over.  Can we add your Reviewed-by?  (See [1]
> for what this means.)

Well, I'd like to see the following occurrence of the same problem
solved in that patch, too. Beyond that - your welcome.

>>
>> Documentation/git-format-patch.txt:  `--subject-prefix` option) has ` v` 
>> appended to it.  E.g.
>>
>> But here the space IS relevant but asciidoc does not pick up
>> the formatting. Perhaps that one could read like this:
>>
>>  `--subject-prefix` option) has `v` appended to it.  E.g.

...

> In some output formats, the text with backticks surrounding it is
> shown in a different background color, which makes something like
> `{space}v` tempting (with appropriate definition of {space} in
> the attributes section of asciidoc.conf).  But that feels way too
> subtle.
> 
> How about something like
> 
>   has a space and `v` appended to it

Well, in the original text "" is already used as a replacement
marker. Therefore "" seemed obvious to me as another replacement
marker which avoids exactly that problem.

> 
> ?
> 
> Thanks,
> Jonathan
> 
> [1] 
> https://kernel.googlesource.com/pub/scm/linux/kernel/git/torvalds/linux/+/9cd6681cb1169e815c41af0265165dd1b872f228/Documentation/process/submitting-patches.rst#563
> 

Re: [PATCH v2 7/9] connect: tell server that the client understands v1

[Brandon Williams]

On 09/27, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > Teach the connection logic to tell a serve that it understands protocol
> > v1.  This is done in 2 different ways for the built in protocols.
> >
> > 1. git://
> >A normal request is structured as "command path/to/repo\0host=..\0"
> >and due to a bug in an old version of git-daemon 73bb33a94 (daemon:
> >Strictly parse the "extra arg" part of the command, 2009-06-04) we
> >aren't able to place any extra args (separated by NULs) besides the
> >host.  In order to get around this limitation put protocol version
> >information after a second NUL byte so the request is structured
> >like: "command path/to/repo\0host=..\0\0version=1\0".  git-daemon can
> >then parse out the version number and set GIT_PROTOCOL.
> 
> Same question as a previous step, wrt the cited commit.  It reads as
> if we are saying that the commit introduced a bug and left it there,
> that we cannot use \0host=..\0version=..\0other=..\0 until that bug
> is fixed, and that in the meantime we use \0host=..\0\0version=.. as
> a workaround, but that reading leaves readers wondering if we want
> to eventually drop this double-NUL workaround.  I am guessing that
> we want to declare that the current protocol has a glitch that
> prevents us to use \0host=..\0version=..\0 but we accept that and
> plan to keep it that way, and we'll use the double-NUL for anything
> other than host from now on, as it is compatible with the current
> version of Git before this patch (the extras are safely ignored),
> but then it still leaves readers wonder if the mention of the
> old commit from 2009 means that this double-NUL would not even work
> if the other end is running a version of Git before that commit, or
> we are safe to talk with versions of Git even older than that.
> 
> I do not think it is a showstopper if we did not work with v1.6.4,
> but it still needs to be clarified.

I wrote an updated commit msg for the daemon change, I can make a
similar change here.  And this mechanism shouldn't cause any issues with
both the pre and post 73bb33a94 git-daemon servers.

> 
> > 2. ssh://, file://
> >Set GIT_PROTOCOL envvar with the desired protocol version.  The
> >envvar can be sent across ssh by using '-o SendEnv=GIT_PROTOCOL' and
> >having the server whitelist this envvar.
> 
> OpenSSH lets us do this, but I do not know how well this works with
> other implementations of SSH clients.  The log message perhaps needs
> to ask for volunteers to check if it is OK with the implementations
> they use, and offer conditional code (just like we have for putty
> and plink customizations) otherwise.

I'll make a comment indicating that

> 
> Other than that, the code changes looked good.
> 
> > diff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh
> > new file mode 100755
> > index 0..1988bbce6
> > --- /dev/null
> > +++ b/t/t5700-protocol-v1.sh
> > @@ -0,0 +1,223 @@
> > +#!/bin/sh
> > +
> > +test_description='test git wire-protocol transition'
> > +
> > +TEST_NO_CREATE_REPO=1
> > +
> > +. ./test-lib.sh
> > +
> > +# Test protocol v1 with 'git://' transport
> > +#
> > +. "$TEST_DIRECTORY"/lib-git-daemon.sh
> > +start_git_daemon --export-all --enable=receive-pack
> > +daemon_parent=$GIT_DAEMON_DOCUMENT_ROOT_PATH/parent
> > +
> > +test_expect_success 'create repo to be served by git-daemon' '
> > +   git init "$daemon_parent" &&
> > +   test_commit -C "$daemon_parent" one
> > +'
> > +
> > +test_expect_success 'clone with git:// using protocol v1' '
> > +   GIT_TRACE_PACKET=1 git -c protocol.version=1 \
> > +   clone "$GIT_DAEMON_URL/parent" daemon_child 2>log &&
> > +
> > +   git -C daemon_child log -1 --format=%s >actual &&
> > +   git -C "$daemon_parent" log -1 --format=%s >expect &&
> > +   test_cmp expect actual &&
> > +
> > +   # Client requested to use protocol v1
> > +   grep "version=1" log &&
> > +   # Server responded using protocol v1
> > +   grep "clone< version 1" log
> 
> This looked a bit strange to check "clone< version 1" for one
> direction, but did not check "$something> version 1" for the other
> direction.  Doesn't "version=1" end up producing 2 hits?

I think you discovered this in your next email but the "version=1" check
is to check for the request sent to git-daemon, the "command
path/to/repo\0host=blah\0\0version=1\0" one. While the "clone< version
1" check is to make sure that the server responded with the correct
version.

> 
> Not a complaint, but wondering if we can write it in such a way that
> does not have to make readers wonder.
> 
> > +'
> > +
> > +test_expect_success 'fetch with git:// using protocol v1' '
> > +   test_commit -C "$daemon_parent" two &&
> > +
> > +   GIT_TRACE_PACKET=1 git -C daemon_child -c protocol.version=1 \
> > +   fetch 2>log &&
> > +
> > +   git -C daemon_child log -1 --format=%s FETCH_HEAD >actual &&
> > +   git -C "$daemon_parent" log -1 --format=%s >expect 

Re: [PATCH Outreachy] mru: use double-linked list from list.h

[Jeff King]

On Fri, Sep 29, 2017 at 06:56:28AM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > But I also think this patch may be a stepping stone to dropping "struct
> > mru" entirely, and just pushing a "struct list_head mru" into the
> > packed_git object itself (or of course any object you like). At which
> > point we'd just directly use the list iterators anyway.
> 
> The endgame state implied by your statement would mean that we won't
> have mru_mark() and instead have these open-coded in terms of the
> two calls into the list API.  There only are two callers of it in
> the current system, so it is not the end of the world, so I guess I
> can agree that this is a good preparation step toward the longer
> term goal, which says "mru API is over-engineered and what it offers
> over the plain vanilla list API is almost never used except for a
> few callsite; let's remove it and use the bare list API instead".

Thanks, that last sentence is a good summary of my thinking (I think
what I find most silly about it is that it allocates a whole extra
struct per item, which is where most of the complication comes from).

I had envisioned leaving mru_mark() as a wrapper for "move to the front"
that could operate on any list. But seeing how Olga's patch takes it
down to two trivial lines, I'd also be fine with an endgame that just
eliminates it.

> > I'd make the same claims here as above (both that I agree your proposed
> > interface looks nicer, but also that I think we eventually do want to
> > expose that this is tightly coupled with list.h).
> 
> I agree.  I just do not yet know if I fully embrace the idea that we
> just should use bare list API, getting rid of the mru API.

Fair enough.

-Peff

Re: [PATCH v2 6/9] connect: teach client to recognize v1 server response

[Brandon Williams]

On 09/27, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > +/* Returns 1 if packet_buffer is a protocol version pkt-line, 0 otherwise. 
> > */
> > +static int process_protocol_version(void)
> > +{
> > +   switch (determine_protocol_version_client(packet_buffer)) {
> > +   case protocol_v1:
> > +   return 1;
> > +   case protocol_v0:
> > +   return 0;
> > +   default:
> > +   die("server is speaking an unknown protocol");
> > +   }
> > +}
> 
> For the purpose of "technology demonstration" v1 protocol, it is OK
> to discard the result of "determine_pvc()" like the above code, but
> in a real application, we would do a bit more than just ignoring an
> extra "version #" packet that appears at the beginning, no?
> 
> It would be sensible to design how the result of determien_pvc()
> call is propagated to the remainder of the program in this patch and
> implement it.  Perhaps add a new global (like server_capabilities
> already is) and store the value there, or something?  Or pass a
> pointer to enum protocol_version as a return-location parameter to
> this helper function so that the process_capabilities() can pass a
> pointer to its local variable?

Yes, once we actually implement a v2 we would need to not throw away the
result of 'determine_pvc()' and instead do control flow based on the
resultant version.  I was trying to implement 'v1' as simply as possible
so that I wouldn't have to do a large amount of refactoring when
proposing this transition, though it seems Jonathan ended up doing more
than I planned, as I figured we didn't really know what the code will
need to be refactored to, in order to handle another protocol version.
I would suspect that we maybe wouldn't want to determine which version a
server is speaking in 'get_remote_heads()' but rather at some point
before that so we can branch off to do v2 like things, for example,
capability discovery and not ref discovery.

If you do think we need to do more of that refactoring now, before a v2,
I can most certainly work on that.


> 
> >  static void process_capabilities(int *len)
> >  {
> > @@ -224,12 +239,19 @@ struct ref **get_remote_heads(int in, char *src_buf, 
> > size_t src_len,
> >  */
> > int responded = 0;
> > int len;
> > -   int state = EXPECTING_FIRST_REF;
> > +   int state = EXPECTING_PROTOCOL_VERSION;
> >  
> > *list = NULL;
> >  
> > while ((len = read_remote_ref(in, _buf, _len, ))) {
> > switch (state) {
> > +   case EXPECTING_PROTOCOL_VERSION:
> > +   if (process_protocol_version()) {
> > +   state = EXPECTING_FIRST_REF;
> > +   break;
> > +   }
> > +   state = EXPECTING_FIRST_REF;
> > +   /* fallthrough */
> > case EXPECTING_FIRST_REF:
> > process_capabilities();
> > if (process_dummy_ref()) {

-- 
Brandon Williams

Re: [PATCH] gitk: expand $config_file_tmp before reporting to user

[Junio C Hamano]

Johannes Schindelin  writes:

>> > Max Kirillov  writes:
>> >
>> >> Tilda-based path may confise some users. First, tilda is not known
>> >> for Window users, second, it may point to unexpected location
>> >> depending on various environment setup.
> ...
> As familiar, as it is unfamiliar to Windows users.
>
> So I would actually suggest to make this a conditional on the platform: on
> Windows, use the native name, everywhere else, not.
>
> Sound good?

Not really.  

I agree with and (more importantly) consider the second rationale
Max cites a more relevant one for this change.  

This is about reporting an error, and using the short-hand ~/$rest
that could be pointing at a location different from what the user
_thinks_ it points at for whatever reason (miconfigured HOME, some
intermediate process setting it to something else, etc.) can hide
the real issue.  The problem can be more easily noticed and
diagnosed if the message shows the result of 'nativename'.  

And that rationale holds whether you are seeing the error message on
Windows or non-Windows.

Re: [PATCH v2 3/9] protocol: introduce protocol extention mechanisms

[Brandon Williams]

On 09/27, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > +`GIT_PROTOCOL`::
> > +   For internal use only.  Used in handshaking the wire protocol.
> > +   Contains a colon ':' separated list of keys with optional values
> > +   'key[=value]'.  Presence of unknown keys must be tolerated.
> 
> Is this meant to be used only on the "server" end?  Am I correct to
> interpret "handshaking" to mean the initial connection acceptor
> (e.g. "git daemon") uses it to pass what it decided to the programs
> that implement the service (e.g. "git receive-pack")?

Yes, the idea is that the client will request a protocol version by
setting GIT_PROTOCOL (or indirectly set when using git:// or http://).
upload-pack and receive-pack will use the keys and values set in
GIT_PROTOCOL to determine which version of the protocol to use.  At some
point in the future they may even use other keys and values as a means
of sending more information in an initial request from the client.

> 
> > +/*
> > + * Environment variable used in handshaking the wire protocol.
> > + * Contains a colon ':' separated list of keys with optional values
> > + * 'key[=value]'.  Presence of unknown keys must be tolerated.
> > + */
> > +#define GIT_PROTOCOL_ENVIRONMENT "GIT_PROTOCOL"
> 
> "Must be tolerated" feels a bit strange.  When somebody asks you to
> use "version 3" or "version 1 variant 2", when you only know
> "version 0" or "version 1" and you are not yet even aware of the
> concept of "variant", we simply ignore "variant=2" as if it wasn't
> there, even though "version=3" will be rejected (because we know of
> "version"; it's just that we don't know "version=3").

By "Must be tolerated" I was trying to get across that if the server
seeing something it doesn't understand, it shouldn't choke.  Maybe a
better wording would be to use the word "ignored"?

> 
> > +enum protocol_version determine_protocol_version_server(void)
> > +{
> > +   const char *git_protocol = getenv(GIT_PROTOCOL_ENVIRONMENT);
> > +   enum protocol_version version = protocol_v0;
> > +
> > +   if (git_protocol) {
> > +   struct string_list list = STRING_LIST_INIT_DUP;
> > +   const struct string_list_item *item;
> > +   string_list_split(, git_protocol, ':', -1);
> > +
> > +   for_each_string_list_item(item, ) {
> > +   const char *value;
> > +   enum protocol_version v;
> > +
> > +   if (skip_prefix(item->string, "version=", )) {
> > +   v = parse_protocol_version(value);
> > +   if (v > version)
> > +   version = v;
> > +   }
> > +   }
> > +
> > +   string_list_clear(, 0);
> > +   }
> > +
> > +   return version;
> > +}
> 
> This implements "the largest one wins", not "the last one wins".  Is
> there a particular reason why the former is chosen?
> 

I envision this logic changing for newer servers once more protocol
versions are added because at some point a server may want to disallow a
particular version (because of a security issue or what have you).  So I
figured the easiest thing to do for now was to implement "Newest version
wins".

-- 
Brandon Williams

Re: [PATCH Outreachy] mru: use double-linked list from list.h

[Junio C Hamano]

Jeff King  writes:

> But I also think this patch may be a stepping stone to dropping "struct
> mru" entirely, and just pushing a "struct list_head mru" into the
> packed_git object itself (or of course any object you like). At which
> point we'd just directly use the list iterators anyway.

The endgame state implied by your statement would mean that we won't
have mru_mark() and instead have these open-coded in terms of the
two calls into the list API.  There only are two callers of it in
the current system, so it is not the end of the world, so I guess I
can agree that this is a good preparation step toward the longer
term goal, which says "mru API is over-engineered and what it offers
over the plain vanilla list API is almost never used except for a
few callsite; let's remove it and use the bare list API instead".

> I'd make the same claims here as above (both that I agree your proposed
> interface looks nicer, but also that I think we eventually do want to
> expose that this is tightly coupled with list.h).

I agree.  I just do not yet know if I fully embrace the idea that we
just should use bare list API, getting rid of the mru API.

EWRGHJUYT

[Sgt Monica Brown]

-- 
Hello,

I bid you greetings, I have an important overture for you. Details shall be 
given when you confirm the receipt of this email.

Regards,
Sgt Monica Brown

[Bug/Solution] Git hangs in compat/poll/poll.c on HPE NonStop

[Randall S. Becker]

Hi Team,

After a whole lot of investigating, we (it is a large "we") have discovered
the reason for the hang we occasionally get in git-upload-pack on HPE
NonStop servers - reported here well over a year ago. This resulted from a
subtle check that the operating system does on file descriptors. When it
sees random values in pfd[i].revents, it sometimes thinks its dealing with a
TTY and well, things end badly after that. There is a non-destructive fix
that I would like to propose for this that I have already tested. Sadly, I
have no email mechanism where our repo resides for a real patch message. The
patch is based on 2.3.7 (16018ae), but should be applicable forward. We have
held off moving to a more recent version until resolving this, so that's
next on our plan.

--- a/compat/poll/poll.c
+++ b/compat/poll/poll.c
@@ -438,6 +438,10 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
pfd[i].revents = happened;
rc++;
  }
+else
+  {
+pfd[i].revents = 0;
+  }
   }

   return rc;

Sincerely,
Randall

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

Re: hacktoberfest

[Jeff King]

On Wed, Sep 27, 2017 at 11:05:49PM +0100, pedro rijo wrote:

> While the git repository itself is not hosted under GitHub, the Pro
> Git book, git for Windows, and git-scm website (at least) projects
> are, and could use this movement to get some more contributions, and
> eventually more maintainers (at least git-scm website had some
> maintainers problem some time ago).
> 
> I've been helping on the git-scm repository (mostly filtering issues
> and PRs), and I know there are still some issues which need to be
> addressed. If the remaining maintainers agree, we could filter and
> provide more instructions to some easy (or not so easy) issues, adding
> the 'hacktoberfest' label and try to use this movement to solve some
> problems

I'd love it if more people wanted to contribute to the git-scm
repository. I think one can probably find some low-hanging fruit by
looking at the open issues list (though I'd be happy, too, if people
with bug or feature suggestions opened new issues).

Here are a couple small-to-moderate bugs that have been languishing:

  https://github.com/git/git-scm.com/issues/701

  https://github.com/git/git-scm.com/issues/987

  https://github.com/git/git-scm.com/issues/994

-Peff

Re: [PATCH v2 3/9] protocol: introduce protocol extention mechanisms

[Brandon Williams]

On 09/26, Stefan Beller wrote:
> > +extern enum protocol_version get_protocol_version_config(void);
> > +extern enum protocol_version determine_protocol_version_server(void);
> > +extern enum protocol_version determine_protocol_version_client(const char 
> > *server_response);
> 
> It would be cool to have some documentation here.

Thanks for reminding me, I'll get to writing some more documentation :)

-- 
Brandon Williams

Re: [PATCH Outreachy] mru: use double-linked list from list.h

[Jeff King]

On Thu, Sep 28, 2017 at 08:38:39AM +, Olga Telezhnaya wrote:

> Simplify mru.c, mru.h and related code by reusing the double-linked
> list implementation from list.h instead of a custom one.

The commit message is a good reason to talk about why we want to do
this. In this case, the answer may be fairly obvious. But I sometimes
find that things that are obvious to me as the patch author are not
quite as obvious to people reading it later (either reviewing, or six
months from now when they are hunting the cause of a bug).

> -void mru_mark(struct mru *mru, struct mru_entry *entry)
> +void mru_mark(struct mru *head, struct mru *entry)
>  {
> - /* If we're already at the front of the list, nothing to do */
> - if (mru->head == entry)
> - return;
> -
> - /* Otherwise, remove us from our current slot... */
> - if (entry->prev)
> - entry->prev->next = entry->next;
> - if (entry->next)
> - entry->next->prev = entry->prev;
> - else
> - mru->tail = entry->prev;
> -
> - /* And insert us at the beginning. */
> - entry->prev = NULL;
> - entry->next = mru->head;
> - if (mru->head)
> - mru->head->prev = entry;
> - mru->head = entry;
> + /* To mark means to put at the front of the list. */
> + list_del(>list);
> + list_add(>list, >list);
>  }

Nice, this hunk is very satisfying. :)

> -void mru_clear(struct mru *mru)
> +void mru_clear(struct mru *head)
>  {
> - struct mru_entry *p = mru->head;
> -
> - while (p) {
> - struct mru_entry *to_free = p;
> - p = p->next;
> + struct list_head *p1;
> + struct list_head *p2;
> + struct mru *to_free;
> + 
> + list_for_each_safe(p1, p2, >list) {
> + to_free = list_entry(p1, struct mru, list);
>   free(to_free);
>   }
> - mru->head = mru->tail = NULL;
> + INIT_LIST_HEAD(>list);

Two minor style comments here:

  - Perhaps "tmp" is a better name than "p2" for the second argument of
a list_for_each_safe, as it makes it less likely to confuse p1 and
p2 (though admittedly the whole function is short enough that it
probably doesn't matter much either way).

  - It's a good practice to declare variables in the smallest scope
possible. So I think the declaration of to_free could go inside the
loop.

You could actually get rid of it entirely with:

  free(list_entry(p1, struct mru, list));

but I certainly don't mind using a variable for better readability.

> @@ -29,17 +28,13 @@
>   * you will begin traversing the whole list again.
>   */
>  
> -struct mru_entry {
> - void *item;
> - struct mru_entry *prev, *next;
> -};
> -
>  struct mru {
> - struct mru_entry *head, *tail;
> + struct list_head list;
> +void *item;
>  };

The decision to get rid of the "mru versus mru_entry" distinction
surprised me a little. In the original, a "struct mru" represented the
whole list. In the list.h implementation, a "struct list_head" serves
that purpose, as a sentinel value. But that sentinel doesn't need to
have an "item", right? I.e., we could have:

  struct mru {
  struct list_head head;
  };

  struct mru_entry {
  void *item;
  struct list_head list;
  };

As I said in my response to Junio (and as we discussed a little
off-list), I think we can eventually move to having no structs at all
(just list_heads embedded inside the existing packfile objects). At
which point the user of the API would just declare:

  LIST_HEAD(packed_git_mru);

themselves. So I'm actually fine with this direction if we're using it
as the "middle step" that I mentioned there.

>  struct mru {
> - struct mru_entry *head, *tail;
> + struct list_head list;
> +void *item;
>  };

The funny indentation in this diff shows that "void *item" is indented
with spaces, not a tab.

> [...]

I pointed out a few minor bits, but overall this is looking very strong.
Great work!

-Peff

Re: [PATCH] fast-import: checkpoint: dump branches/tags/marks even if object_count==0

[Eric Rannaud]

On Thu, Sep 28, 2017 at 5:59 AM, Adam Dinwoodie  wrote:
> On Wed, Sep 27, 2017 at 10:07:41PM -0700, Eric Rannaud wrote:
>>
>> Also adding the necessary PIPE prereq, as pointed out by Ramsay Jones.
>
> Cygwin doesn't have the PIPE prereq; I've just confirmed that the
> previous version of this patch has t9300 failing on Cygwin, but this
> version passes.

What's the preferred solution here? I can avoid using named pipes entirely:

read_checkpoint () {
if read output
then
if ! test "$output" = "progress checkpoint"
then
echo >&2 "no progress checkpoint received: 
$output"
echo 1 > V.result
else
echo 0 > V.result
fi
else
echo >&2 "failed to read fast-import output"
echo 1 > V.result
fi
}

# The commands in input_file should not produce any output on the file
# descriptor set with --cat-blob-fd (or stdout if unspecified).
#
# To make sure you're observing the side effects of checkpoint *before*
# fast-import terminates (and thus writes out its state), check that the
# fast-import process is still running using 
background_import_still_running
# *after* evaluating the test conditions.
background_import_then_checkpoint () {
options=$1
input_file=$2

rm -f V.result

( cat "$input_file"
echo "checkpoint"
echo "progress checkpoint"
sleep 3600 &
echo $! >V.pid
wait ) | git fast-import $options | read_checkpoint &

# We don't mind if the pipeline has already died by the time 
the test
# ends.
test_when_finished "kill $(cat V.pid) || true"

while ! test -f V.result
do
# Try to sleep less than a second, if supported.
sleep .1 2>/dev/null || sleep 1
done
return $(cat V.result)
}

Do we like this better?

Re: [PATCH Outreachy] mru: use double-linked list from list.h

[Jeff King]

On Thu, Sep 28, 2017 at 08:03:00PM +0900, Junio C Hamano wrote:

> > -   for (entry = packed_git_mru.head; entry; entry = entry->next) {
> > +   list_for_each(pos, _git_mru.list) {
> > +   struct mru *entry = list_entry(pos, struct mru, list);
> > struct packed_git *p = entry->item;
> > off_t offset;
> 
> I was a bit surprised to see a change outside mru.[ch] like this
> one.  The reason why I was surprised was because I expected mru.[ch]
> would offer its own API that encapsulates enumeration like this one
> and this patch would just be reimplementing that API using the list
> API, instead of rewriting the users of mru API to directly access
> the list API.
> 
> Alas, there is no such mru API that lets a mru user to iterate over
> elements, so the original of the above code were using mru's
> implementation detail directly.
> 
> We probably want to invent mru_for_each() that hides the fact that
> mru is implemented in terms of list_head from the users of mru API
> and use it here.

I agree that the caller would be a little shorter with an mru-specific
iterator (e.g., we could probably do the list_entry() part
automatically).

But I also think this patch may be a stepping stone to dropping "struct
mru" entirely, and just pushing a "struct list_head mru" into the
packed_git object itself (or of course any object you like). At which
point we'd just directly use the list iterators anyway.

(One could argue that if that's our end goal, we could go straight
there. But I think this middle state has value, because the individual
steps are easier to verify).

> > @@ -8,18 +10,15 @@
> >   *
> >   *   // Create a list.  Zero-initialization is required.
> >   *   static struct mru cache;
> > - *   mru_append(, item);
> > - *   ...
> > + *   INIT_LIST_HEAD();
> 
> "Zero-initialization is required." is no longer true, it seems, and
> the comment above needs to be updated, right?
> 
> More importantly, this leaks to the user of the API the fact that
> mru is internally implemented in terms of the list API, which is
> not necessary (when we want to update the implementation later, we'd
> need to update all the users again).  Perhaps you'd want
> 
>   INIT_MRU(cache);
> 
> which is #define'd in this file, perhaps like so:
> 
>   #define INIT_MRU(mru)   INIT_LIST_HEAD(&((mru).list))

I'd make the same claims here as above (both that I agree your proposed
interface looks nicer, but also that I think we eventually do want to
expose that this is tightly coupled with list.h).

-Peff

Re: [PATCH 1/1] fast-import: checkpoint: dump branches/tags/marks even if object_count==0

[Eric Rannaud]

On Thu, Sep 28, 2017 at 3:35 AM, Junio C Hamano  wrote:
> "Eric Rannaud"  writes:
>
>> +# The commands in input_file should not produce any output on the file
>> +# descriptor set with --cat-blob-fd (or stdout if unspecified).
>
> Thanks for documenting this.  Swapping the order of starting
> fast-import and feeding its input (which is one change in this
> version relative to the previous one) alone would not help, because
> in the updated order in this patch, nobody is reading from
> fast-import until the parent process finishes feeding it.

Darn. That's correct, on a platform with blocking pipes it would not
work. I will start the input cat in the background (so it may block
while writing), before reading from the output of fast-import.

Re: [PATCH] oidmap: map with OID as key

[Jeff King]

On Thu, Sep 28, 2017 at 10:46:16AM -0700, Jonathan Tan wrote:

> > To me it seems like a much simpler API for a map would be to just allow
> > callers to store a 'void *' as the value.
> 
> I agree that the API would be simpler.
> 
> My main motivation with this design is indeed to save memory, and not
> inconvenience the user too much (in the case where you're storing things
> larger than one pointer, you just need to remember to put the special
> struct at the beginning of your struct), but if memory is not so
> important, I agree that we can switch to the "util" design.

When I saw that you were implementing "oidset" in terms of "oidmap", I
was all ready to be crabby about this extra memory. But then I saw that
the implementation tries hard not to waste any memory. :)

All of which is to say I gave this some thought when I was in the "ready
to be crabby" phase, and came to the conclusion that it probably isn't
that painful. An unused pointer is 8 bytes per entry. We're already
spending 20 for the oid itself (which is likely to grow to 32
eventually), plus 8 for the chained "next" pointer. Plus potentially 8
for a padded version of the hash, if we just use a straight hashmap that
duplicates the hash field.

So depending how you count it, we're wasting between 28% (sha1 and no
extra hash) and 16% (sha256 plus reusing hashmap). That's not great, but
it's probably not breaking the bank.

Another way of thinking about it. Large-ish (but not insane) repos have
on the order of 5-10 million objects. If we had an oidset that mentioned
every single object in the repository, that's 40-80MB wasted in the
worst case. For current uses of oidset, that's probably fine. It's
generally used only to collect ref tips (so probably two orders of
magnitude less).

If you're planning on using an oidset to mark every object in a
100-million-object monorepo, we'd probably care more. But I'd venture to
say that any scheme which involves generating that hash table on the fly
is doing it wrong. At at that scale we'd want to look at compact
mmap-able on-disk representations.

So I think we may be better off going with the solution here that's
simpler and requires introducing less code. If it does turn out to be a
memory problem in the future, this is a _really_ easy thing to optimize
after the fact, because we have these nice abstractions.

-Peff

Re: [PATCH] doc: correct command formatting

[Jonathan Nieder]

Hi,

Andreas Heiduk wrote:

> +1, Thanks for spotting.

Thanks for looking it over.  Can we add your Reviewed-by?  (See [1]
for what this means.)

> I did a quick
>
>   grep -r " ` "
>
> which came up with with another relevant place:
>
> Documentation/git-format-patch.txt:   `--subject-prefix` option) has ` v` 
> appended to it.  E.g.
>
> But here the space IS relevant but asciidoc does not pick up
> the formatting. Perhaps that one could read like this:
>
>   `--subject-prefix` option) has `v` appended to it.  E.g.

Interesting.  This comes from

  commit 4aad08e061df699b49e24c4d34698d734473fb66
  Author: Junio C Hamano 
  Date:   Wed Jan 2 14:16:07 2013 -0800

  format-patch: document and test --reroll-count

In some output formats, the text with backticks surrounding it is
shown in a different background color, which makes something like
`{space}v` tempting (with appropriate definition of {space} in
the attributes section of asciidoc.conf).  But that feels way too
subtle.

How about something like

has a space and `v` appended to it

?

Thanks,
Jonathan

[1] 
https://kernel.googlesource.com/pub/scm/linux/kernel/git/torvalds/linux/+/9cd6681cb1169e815c41af0265165dd1b872f228/Documentation/process/submitting-patches.rst#563

important/urgent

[Khadov Amir Moh.]

--
Hello

I am Khadov Amir Moh., a former resident of Birmingham UK.
Please reach me for the details of an extremely important business
proposition.

Have a nice day.
Khadov Amir Moh.

Re: [PATCH] doc: correct command formatting

[Jonathan Nieder]

Adam Dinwoodie wrote:

> Leaving spaces around the `-delimeters for commands means asciidoc fails
> to parse them as the start of a literal string.  Remove an extraneous
> space that is causing a literal to not be formatted as such.
>
> Signed-off-by: Adam Dinwoodie 
> ---
>  Documentation/git.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Good catch.

Reviewed-by: Jonathan Nieder 

Re: [PATCH] oidmap: map with OID as key

[Jonathan Tan]

On Wed, 27 Sep 2017 17:41:37 -0700
Brandon Williams  wrote:

> On 09/27, Jonathan Tan wrote:
> > This is similar to using the hashmap in hashmap.c, but with an
> > easier-to-use API. In particular, custom entry comparisons no longer
> > need to be written, and lookups can be done without constructing a
> > temporary entry structure.
> > 
> > oidset has been updated to use oidmap.
> 
> After a quick glance at the code, I think it looks good.  I do have one
> suggestion though.  This map is structured much like our internal
> hashmap where if you want to include any data you have to implement your
> own struct with the internal 'entry' struct as the first member in your
> custom struct.  I personal dislike this method, despite the potential
> memory savings, because it makes the data structure much more difficult
> to use.
> 
> If all i wanted was a map from 'OID -> const char *' then I would need
> to write all the boilerplate code to make a struct which contains the
> map 'entry' struct + the 'const char *' entry.
>
> You are also making the
> caller responsible for allocating the individual entries instead of
> letting the data structure take care of that internally.

In the case where your extra data ("const char *" in your example) fits
in a pointer, yes it's true that the "util" design eliminates the need
to define and allocate a struct. But if you need to store more than
that, you will still have that need.

> To me it seems like a much simpler API for a map would be to just allow
> callers to store a 'void *' as the value.

I agree that the API would be simpler.

My main motivation with this design is indeed to save memory, and not
inconvenience the user too much (in the case where you're storing things
larger than one pointer, you just need to remember to put the special
struct at the beginning of your struct), but if memory is not so
important, I agree that we can switch to the "util" design.

Re: [PATCH] oidmap: map with OID as key

[Jonathan Tan]

On Thu, 28 Sep 2017 12:13:00 +0900
Junio C Hamano  wrote:

> Jonathan Tan  writes:
> 
> > This is similar to using the hashmap in hashmap.c, but with an
> > easier-to-use API. In particular, custom entry comparisons no longer
> > need to be written, and lookups can be done without constructing a
> > temporary entry structure.
> 
> A naïve question is why this needs to duplicate so much code, just
> to build something similar in spirit to hashmap but unlike hashmap
> that can take caller-defined keys, limited to using oid as the keys,
> instead of just being a thin API wrapper that uses hashmap as its
> internal implementation detail.  
> 
> Is the way hashmap API is structured so hard to use it in such a
> way, or something?

Another reason that I probably should have mentioned is the opportunity
to save 4 bytes. I didn't mention it in the commit message at first
because I felt that the benefit was very specific and, as far as I know,
wouldn't be seen on architectures with 8-byte-aligned pointers until we
change the hash function. But I should have probably written something
like this in the commit message:

Using oidmap also saves 4 bytes per entry, compared to hashmap,
because the hash does not need to be stored separately as an int.
(However, alignment considerations might mean that we will not
observe these savings until we change the hash function, since
currently 20-byte hashes are being used.)

If we decide that the 4 bytes are not important, right now at least, I
can change it so that this is a thin API wrapper over hashmap. We could
always change it later.

Re: [PATCH] doc: correct command formatting

[Andreas Heiduk]

Am 28.09.2017 um 16:06 schrieb Adam Dinwoodie:
> Leaving spaces around the `-delimeters for commands means asciidoc fails
> to parse them as the start of a literal string.  Remove an extraneous
> space that is causing a literal to not be formatted as such.
> 
> Signed-off-by: Adam Dinwoodie 
> ---
>  Documentation/git.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index 6e3a6767e..98b9b46b9 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -75,7 +75,7 @@ example the following invocations are equivalent:
>  Note that omitting the `=` in `git -c foo.bar ...` is allowed and sets
>  `foo.bar` to the boolean true value (just like `[foo]bar` would in a
>  config file). Including the equals but with an empty value (like `git -c
> -foo.bar= ...`) sets `foo.bar` to the empty string which ` git config
> +foo.bar= ...`) sets `foo.bar` to the empty string which `git config
>  --bool` will convert to `false`.
>  
>  --exec-path[=]::
> 

+1, Thanks for spotting.

I did a quick

grep -r " ` "

which came up with with another relevant place:

Documentation/git-format-patch.txt: `--subject-prefix` option) has ` v` 
appended to it.  E.g.

But here the space IS relevant but asciidoc does not pick up
the formatting. Perhaps that one could read like this:


`--subject-prefix` option) has `v` appended to it.  E.g.

Re: [RFC] clang-format: outline the git project's coding style

[Brandon Williams]

On 09/28, Johannes Schindelin wrote:
> Hi all,
>
> On Thu, 10 Aug 2017, Johannes Schindelin wrote:
>
> > On Tue, 8 Aug 2017, Brandon Williams wrote:
> >
> > > On 08/08, Stefan Beller wrote:
> > > > On Tue, Aug 8, 2017 at 5:05 AM, Johannes Schindelin
> > > >  wrote:
> > > > >
> > > > > On Mon, 7 Aug 2017, Brandon Williams wrote:
> > > > >
> > > > >> Add a '.clang-format' file which outlines the git project's
> > > > >> coding style.  This can be used with clang-format to auto-format
> > > > >> .c and .h files to conform with git's style.
> > > > >>
> > > > >> Signed-off-by: Brandon Williams 
> > > > >> ---
> > > > >>
> > > > >> I'm sure this sort of thing comes up every so often on the list
> > > > >> but back at git-merge I mentioned how it would be nice to not
> > > > >> have to worry about style when reviewing patches as that is
> > > > >> something mechanical and best left to a machine (for the most
> > > > >> part).
> > > > >
> > > > > Amen.
> > > > >
> > > > > If I never have to see a review mentioning an unwrapped line, I am 
> > > > > quite
> > > > > certain I will be quite content.
> > > >
> > > > As a thought experiment I'd like to propose to take it one step further:
> > > >
> > > >   If the code was formatted perfectly in one style such that a
> > > >   formatter for this style would not produce changes when rerun
> > > >   again on the code, then each individual could have a clean/smudge
> > > >   filter to work in their preferred style, and only the exchange and
> > > >   storage of code is in a mutual agreed style. If the mutually
> > > >   agreed style is close to what I prefer, I don't have to use
> > > >   clean/smudge filters.
> > > >
> > > > Additionally to this patch, we'd want to either put a note into
> > > > SubmittingPatches or Documentation/gitworkflows.txt to hint at
> > > > how to use this formatting to just affect the patch that is currently
> > > > worked on or rather a pre-commit hook?
> > >
> > > I'm sure both of these things will probably happen if we decide to make
> > > use of a code-formatter.  This RFC is really just trying to ask the
> > > question: "Is this something we want to entertain doing?"
> > > My response would be 'Yes' but there's many other opinions to consider
> > > first :)
> >
> > I am sure that something even better will be possible: a Continuous
> > "Integration" that fixes the coding style automatically by using
> > `filter-branch` (avoiding the merge conflicts that would arise if `rebase
> > -i` was used).
>
> FWIW I just set this up in my VSTS account, with the following build step
> (performed in one of the Hosted Linux agents, i.e. I do not even have to
> have a VM dedicated to the task):
>
> -- snip --
> die () {
> echo "$*" >&2
> exit 1
> }
>
> head=$(git rev-parse HEAD) ||
> die "Could not determine HEAD"
>
> test -n "$BUILD_SOURCEBRANCHNAME" ||
> die "Need a source branch name to work with"
>
> base=$(git merge-base origin/core/master HEAD) &&
> count=$(git rev-list --count $base..) &&
> test 0 -lt $count ||
> die "Could not determine commits to clean up (count: $count)"
>
> test -f .clang-format ||
> git show origin/core/pu/.clang-format >.clang-format ||
> die "Need a .clang-format"
>
> sudo add-apt-repository 'deb http://apt.llvm.org/xenial/
> llvm-toolchain-xenial main' &&
> sudo apt-get update &&
> sudo apt-get --allow-unauthenticated -y install clang-format-6.0 ||
> die "Could not install clang-format 6.0"
>
> git filter-branch -f --tree-filter \
> 'git diff HEAD^.. |
>  clang-format-diff-6.0 -p 1 -i &&
>
>  git update-index --refresh --ignore-submodules &&
>  git diff-files --quiet --ignore-submodules ||
>  git commit --amend -C HEAD' $base.. ||
> die "Could not rewrite branch"
>
> if test "$head" = "$(git rev-parse HEAD)"
> then
> echo "No changes in $BUILD_SOURCEBRANCHNAME introduced by clang-format" 
> >&2
> else
> git push vsts +HEAD:refs/heads/clang-format/"$BUILD_SOURCEBRANCHNAME" ||
> die "Could not push clang-format/$BUILD_SOURCEBRANCHNAME"
> echo "Clean branch pushed as clang-format/$BUILD_SOURCEBRANCHNAME" >&2
> exit 123
> fi
> -- snap --
>
> A couple of notes for the interested:
>
> - you can easily set this up for yourself, as Visual Studio Team Services
>   is free for small teams up to five people (including single developer
>   "teams"): https://www.visualstudio.com/team-services/, and of course you
>   can back it by your own git.git fork on GitHub, no need to host the code
>   in VSTS.
>
>   Disclaimer: I work closely with the developers behind Visual Studio Team
>   Services, and I am a genuine fan, yet I understand if anybody thinks of
>   this as advertising the service, so this will be the only time I mention
>   this.
>
> - the script assumes that there is a `core/master` tracking upstream Git's
>   master branch, then reformats the commits in the current branch that are
>   not also reachable from core/master.
>
> - The 

Re: [PATCH v2 4/9] daemon: recognize hidden request arguments

[Brandon Williams]

On 09/27, Brandon Williams wrote:
> On 09/27, Junio C Hamano wrote:
> > Brandon Williams  writes:
> >
> > > A normal request to git-daemon is structured as
> > > "command path/to/repo\0host=..\0" and due to a bug in an old version of
> > > git-daemon 73bb33a94 (daemon: Strictly parse the "extra arg" part of the
> > > command, 2009-06-04) we aren't able to place any extra args (separated
> > > by NULs) besides the host.
> >
> > It's a bit unclear if that commit _introduced_ a bug, or just
> > noticed an old bug and documented it in its log message.  How does
> > that commit impact the versons of Git that the updated code is
> > capable of interracting with?
>
> You're right, after reading it again it isn't clear.  I'll change this
> to indicate that the commit is a fix to a bug and that the fix doesn't
> allow us to place any additional args.

Ok how about this wording for the commit msg:

  daemon: recognize hidden request arguments

  A normal request to git-daemon is structured as "command
  path/to/repo\0host=..\0" and due to a bug introduced in
  49ba83fb6 (Add virtualization support to git-daemon, 2006-09-19)
  we aren't able to place any extra arguments (separated by NULs)
  besides the host otherwise the parsing of those arguments would
  enter an infinite loop.  This bug was fixed in 73bb33a94
  (daemon: Strictly parse the "extra arg" part of the command,
  2009-06-04) but a check was put in place to disallow extra
  arguments so that new clients wouldn't trigger this bug in older
  servers.

  In order to get around this limitation teach git-daemon to
  recognize additional request arguments hidden behind a second
  NUL byte.  Requests can then be structured like: "command
  path/to/repo\0host=..\0\0version=1\0key=value\0".  git-daemon
  can then parse out the extra arguments and set 'GIT_PROTOCOL'
  accordingly.

  By placing these extra arguments behind a second NUL byte we can
  skirt around both the infinite loop bug in 49ba83fb6 (Add
  virtualization support to git-daemon, 2006-09-19) as well as the
  explicit disallowing of extra arguments introduced in 73bb33a94
  (daemon: Strictly parse the "extra arg" part of the command,
  2009-06-04) because both of these versions of git-daemon check
  for a single NUL byte after the host argument before terminating
  the argument parsing.

This way I'm citing when the bug was actually introduced as well as
describing why the 'fix' didn't completely resolve the issue.  I also
explain a little bit about how this change should work even with very
old servers which still have the bug.  (I tried to get the introp test
to work on a version of git that old but am having some difficulty even
getting the old version to launch git-daemon without hanging for some
unknown reason)

--
Brandon Williams

Re: [PATCH v2] git: add --no-optional-locks option

[Johannes Schindelin]

Hi Peff,

On Wed, 27 Sep 2017, Jeff King wrote:

> For the other comments on v1, I decided not to make any changes:
> 
>   - JSixt suggested marking this as a local-repo variable. I think we
> really do want it to cross repo boundaries to handle submodules (and
> the GfW version does the same).

Indeed. We had it originally as a local-repo only variable, and you
probably guessed that submodules made us rethink that approach.

>   - there was some discussion of alternatives, including reader/writer
> locking schemes and lock-retry timeouts for other callers. This
> approach has the benefit of simplicity and having been tested in the
> real world by GfW/VS.  I wouldn't mind if somebody wanted to explore
> lock retries, but I think we'd want to have this feature regardless.

We also weighed other options, and took this route for simplicity. It's
possible to explain during an uninterrupted elevator ride what it does.
Makes for less bugs.

And yes, we wanted to test it first before showing it to the Git mailing
list ;-)

Ciao,
Dscho

Re: [PATCH 07/13] object-filter: common declarations for object filtering

[Jeff Hostetler]

On 9/27/2017 8:05 PM, Jonathan Tan wrote:

On Wed, 27 Sep 2017 13:09:42 -0400
Jeff Hostetler  wrote:


On 9/26/2017 6:39 PM, Jonathan Tan wrote:

On Fri, 22 Sep 2017 20:30:11 +
Jeff Hostetler  wrote:


   Makefile|   1 +
   object-filter.c | 269 

   object-filter.h | 173 
   3 files changed, 443 insertions(+)
   create mode 100644 object-filter.c
   create mode 100644 object-filter.h


I think these and list-objects-filter-* are multiple levels of
indirection too many. Would a single file with a few implementations of
filter_object_fn be sufficient?


I did that in my first draft and I found it confusing.

Each filter has 3 parts (some filter-specific data structures,
a filter callback routine, a driver to call the traverse code).
I found it easier to reason about each filter in isolation.
And it makes it easier to work on each independently and keep
their inclusion in separate commits.


I looked at object-filter.{c,h} a bit more. It seems that these files:
  1) define a struct that contains all the options that we would want
  2) supplies a way to populate this struct from code that uses parse-options
  3) supplies a way to populate this struct from code that calculates
 options by hand
  4) supplies a way to populate this struct from "protocol" ("" or
 " " strings)

And the next commit takes the struct that object-filter.{c,h} produces
and actually performs the traversal.

I think this can be significantly simplified, though. Would this work:
  a) Define the object_filter_options struct, but make all fields
 correspond to a single parameter each. Define
 OBJECT_FILTER_OPTIONS_INIT to initialize everything to 0 except for
 large_byte_limit to ULONG_MAX (so that we can detect if something
 else is set to it).
  b) Define one single OPT_PARSE_FILTER macro containing all the
 parameters. We can use the non-callback macros here. That solves 2)
 above.
  c) Define a function that takes in (int *argc, char ***argv) that can
 "massage" it to remove all filter-related arguments, storing them in
 a object_filter_options struct. That solves 3) above. As discussed
 in the API documentation, this means that argument lists of the form
 "--unknown --known" (where "--unknown" takes an argument) are
 processed differently, but then again, rev-list never supported them
 anyway (it required "--unknown=").
  d) Define a function that converts "" into "--" and "
 " into "--=", and use the existing mechanism.
 That solves 4) above.

This removes the need to maintain the lists of one-per-argument
functions, including the parse_filter_* and opt_parse_filter_* functions
declared in the header file. If we were to add a feature, we wouldn't
need to change anything in the caller, and wouldn't need to hand-edit
object_filter_hand_parse_arg() and object_filter_hand_parse_protocol().


Maybe.  What I have here now is the result of adding these arguments to
rev-list and pack-objects (in the current patch series), and also to
fetch-pack, fetch, clone, upload-pack, index-pack, and the transport and
protocol code (in a follow-on patch series that I've omitted for the moment).
And there will probably be a few more, such as fsck, gc, and etc.  I hesitate
to refine the macros too much further until we've agreement on the overall
approach and terms.

Thanks
Jeff

[PATCH] doc: correct command formatting

[Adam Dinwoodie]

Leaving spaces around the `-delimeters for commands means asciidoc fails
to parse them as the start of a literal string.  Remove an extraneous
space that is causing a literal to not be formatted as such.

Signed-off-by: Adam Dinwoodie 
---
 Documentation/git.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 6e3a6767e..98b9b46b9 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -75,7 +75,7 @@ example the following invocations are equivalent:
 Note that omitting the `=` in `git -c foo.bar ...` is allowed and sets
 `foo.bar` to the boolean true value (just like `[foo]bar` would in a
 config file). Including the equals but with an empty value (like `git -c
-foo.bar= ...`) sets `foo.bar` to the empty string which ` git config
+foo.bar= ...`) sets `foo.bar` to the empty string which `git config
 --bool` will convert to `false`.
 
 --exec-path[=]::
-- 
2.14.1

Re: [PATCH 1/1] fast-import: checkpoint: dump branches/tags/marks even if object_count==0

[Adam Dinwoodie]

On Wed, Sep 27, 2017 at 10:07:41PM -0700, Eric Rannaud wrote:
> The checkpoint command cycles packfiles if object_count != 0, a sensible
> test or there would be no pack files to write. Since 820b931012, the
> command also dumps branches, tags and marks, but still conditionally.
> However, it is possible for a command stream to modify refs or create
> marks without creating any new objects.
> 
> For example, reset a branch (and keep fast-import running):
> 
>   $ git fast-import
>   reset refs/heads/master
>   from refs/heads/master^
> 
>   checkpoint
> 
> but refs/heads/master remains unchanged.
> 
> Other example: a commit command that re-creates an object that already
> exists in the object database.
> 
> The man page also states that checkpoint "updates the refs" and that
> "placing a progress command immediately after a checkpoint will inform
> the reader when the checkpoint has been completed and it can safely
> access the refs that fast-import updated". This wasn't always true
> without this patch.
> 
> This fix unconditionally calls dump_{branches,tags,marks}() for all
> checkpoint commands. dump_branches() and dump_tags() are cheap to call
> in the case of a no-op.
> 
> Add tests to t9300 that observe the (non-packfiles) effects of
> checkpoint.
> 
> Signed-off-by: Eric Rannaud 
> ---
>  fast-import.c  |   6 +--
>  t/t9300-fast-import.sh | 126 
> +
>  2 files changed, 129 insertions(+), 3 deletions(-)
> 
> 
> Updated to include Junio's latest remarks.
> 
> Also adding the necessary PIPE prereq, as pointed out by Ramsay Jones.

Cygwin doesn't have the PIPE prereq; I've just confirmed that the
previous version of this patch has t9300 failing on Cygwin, but this
version passes.

Re: [PATCH] gitk: expand $config_file_tmp before reporting to user

[Johannes Schindelin]

Hi Junio,

On Thu, 28 Sep 2017, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> > Max Kirillov  writes:
> >
> >> Tilda-based path may confise some users. First, tilda is not known
> >> for Window users, second, it may point to unexpected location
> >> depending on various environment setup.
> >>
> >> Expand the path to "nativename", so that ~/.config/git/gitk-tmp
> >> would be "C:\Users\user\.config\git\gitk-tmp", for example.
> >> It should be less cryptic

Thanks, Max, for your contribution!

> > It might be less cryptic, but for those of us whose $HOME is a
> > looong path, ~/.config/git/gitk-tmp is much easier to understand
> > than the same path with ~/ expanded, which would push the part of
> > the filename that most matters far to the right hand side of the
> > dialog.

Heh, do you want to know how that must sound to a Windows user? I'm not
saying that I am a hard-core Windows user, but I do know a few, and I
already hear their comments in their own voice in my head...

> > I somehow find this change just robbing Peter to pay Paul.
> 
> Having said that, because a set-up might have HOME or XDG_CONFIG or
> other things misconfigured to point at a place where the end user
> may not be expecting, I tend to think that catering to Paul by
> showing the information closer to the bare metal is much more worthy
> thing to do than keeping Peter happy.  Since this is an error path,
> accuracy trumps convenience.
> 
> So no objection from me (unless somebody else comes up with an
> alternative that would make both camps happy, that is).

To Unix/Linux users, the tilde (or Tilda, I really like that nickname, it
makes it much more human and tolerable) is probably *very* familiar.

As familiar, as it is unfamiliar to Windows users.

So I would actually suggest to make this a conditional on the platform: on
Windows, use the native name, everywhere else, not.

Sound good?
Johannes

Re: [RFC] clang-format: outline the git project's coding style

[Johannes Schindelin]

Hi all,

On Thu, 10 Aug 2017, Johannes Schindelin wrote:

> On Tue, 8 Aug 2017, Brandon Williams wrote:
> 
> > On 08/08, Stefan Beller wrote:
> > > On Tue, Aug 8, 2017 at 5:05 AM, Johannes Schindelin
> > >  wrote:
> > > >
> > > > On Mon, 7 Aug 2017, Brandon Williams wrote:
> > > >
> > > >> Add a '.clang-format' file which outlines the git project's
> > > >> coding style.  This can be used with clang-format to auto-format
> > > >> .c and .h files to conform with git's style.
> > > >>
> > > >> Signed-off-by: Brandon Williams 
> > > >> ---
> > > >>
> > > >> I'm sure this sort of thing comes up every so often on the list
> > > >> but back at git-merge I mentioned how it would be nice to not
> > > >> have to worry about style when reviewing patches as that is
> > > >> something mechanical and best left to a machine (for the most
> > > >> part).
> > > >
> > > > Amen.
> > > >
> > > > If I never have to see a review mentioning an unwrapped line, I am quite
> > > > certain I will be quite content.
> > > 
> > > As a thought experiment I'd like to propose to take it one step further:
> > > 
> > >   If the code was formatted perfectly in one style such that a
> > >   formatter for this style would not produce changes when rerun
> > >   again on the code, then each individual could have a clean/smudge
> > >   filter to work in their preferred style, and only the exchange and
> > >   storage of code is in a mutual agreed style. If the mutually
> > >   agreed style is close to what I prefer, I don't have to use
> > >   clean/smudge filters.
> > > 
> > > Additionally to this patch, we'd want to either put a note into
> > > SubmittingPatches or Documentation/gitworkflows.txt to hint at
> > > how to use this formatting to just affect the patch that is currently
> > > worked on or rather a pre-commit hook?
> > 
> > I'm sure both of these things will probably happen if we decide to make
> > use of a code-formatter.  This RFC is really just trying to ask the
> > question: "Is this something we want to entertain doing?"
> > My response would be 'Yes' but there's many other opinions to consider
> > first :)
> 
> I am sure that something even better will be possible: a Continuous
> "Integration" that fixes the coding style automatically by using
> `filter-branch` (avoiding the merge conflicts that would arise if `rebase
> -i` was used).

FWIW I just set this up in my VSTS account, with the following build step
(performed in one of the Hosted Linux agents, i.e. I do not even have to
have a VM dedicated to the task):

-- snip --
die () {
echo "$*" >&2
exit 1
}

head=$(git rev-parse HEAD) ||
die "Could not determine HEAD"

test -n "$BUILD_SOURCEBRANCHNAME" ||
die "Need a source branch name to work with"

base=$(git merge-base origin/core/master HEAD) &&
count=$(git rev-list --count $base..) &&
test 0 -lt $count ||
die "Could not determine commits to clean up (count: $count)"

test -f .clang-format ||
git show origin/core/pu/.clang-format >.clang-format ||
die "Need a .clang-format"

sudo add-apt-repository 'deb http://apt.llvm.org/xenial/
llvm-toolchain-xenial main' &&
sudo apt-get update &&
sudo apt-get --allow-unauthenticated -y install clang-format-6.0 ||
die "Could not install clang-format 6.0"

git filter-branch -f --tree-filter \
'git diff HEAD^.. |
 clang-format-diff-6.0 -p 1 -i &&

 git update-index --refresh --ignore-submodules &&
 git diff-files --quiet --ignore-submodules ||
 git commit --amend -C HEAD' $base.. ||
die "Could not rewrite branch"

if test "$head" = "$(git rev-parse HEAD)"
then
echo "No changes in $BUILD_SOURCEBRANCHNAME introduced by clang-format" >&2
else
git push vsts +HEAD:refs/heads/clang-format/"$BUILD_SOURCEBRANCHNAME" ||
die "Could not push clang-format/$BUILD_SOURCEBRANCHNAME"
echo "Clean branch pushed as clang-format/$BUILD_SOURCEBRANCHNAME" >&2
exit 123
fi
-- snap --

A couple of notes for the interested:

- you can easily set this up for yourself, as Visual Studio Team Services
  is free for small teams up to five people (including single developer
  "teams"): https://www.visualstudio.com/team-services/, and of course you
  can back it by your own git.git fork on GitHub, no need to host the code
  in VSTS.

  Disclaimer: I work closely with the developers behind Visual Studio Team
  Services, and I am a genuine fan, yet I understand if anybody thinks of
  this as advertising the service, so this will be the only time I mention
  this.

- the script assumes that there is a `core/master` tracking upstream Git's
  master branch, then reformats the commits in the current branch that are
  not also reachable from core/master.

- The push credentials to push the result at the end are of course not
  included in the script, they need to be provided separately.

- the exit code 123 when the branch needed to be rewritten indicates to
  any consumer that the build "failed". The reason is that I want to
  

Re: [PATCH Outreachy] mru: use double-linked list from list.h

[Junio C Hamano]

Olga Telezhnaya  writes:

> Simplify mru.c, mru.h and related code by reusing the double-linked list 
> implementation from list.h instead of a custom one.

An overlong line (I can locally wrap it, so the patch does not have
to be re-sent only to fix this alone).

> Signed-off-by: Olga Telezhnaia 

Thanks for making your "From:" name match the "Signed-off-by:" name;
anglicising like you did is probably more friendly to the readers
than writing both in Cyrillic, which is another valid way to make
them match.

> Mentored-by: Christian Couder 
> Mentored by: Jeff King 
> ---
>  builtin/pack-objects.c |  5 +++--
>  mru.c  | 51 
> +++---
>  mru.h  | 31 +-
>  packfile.c |  6 --
>  4 files changed, 35 insertions(+), 58 deletions(-)

As Christian mentioned earlier, nice line reduction ;-)

> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index f721137eaf881..ba812349e0aab 100644
> --- a/builtin/pack-objects.c
%> +++ b/builtin/pack-objects.c
> @@ -995,8 +995,8 @@ static int want_object_in_pack(const unsigned char *sha1,
>  struct packed_git **found_pack,
>  off_t *found_offset)
>  {
> - struct mru_entry *entry;
>   int want;
> + struct list_head *pos;
>  
>   if (!exclude && local && has_loose_object_nonlocal(sha1))
>   return 0;
> @@ -1012,7 +1012,8 @@ static int want_object_in_pack(const unsigned char 
> *sha1,
>   return want;
>   }
>  
> - for (entry = packed_git_mru.head; entry; entry = entry->next) {
> + list_for_each(pos, _git_mru.list) {
> + struct mru *entry = list_entry(pos, struct mru, list);
>   struct packed_git *p = entry->item;
>   off_t offset;

I was a bit surprised to see a change outside mru.[ch] like this
one.  The reason why I was surprised was because I expected mru.[ch]
would offer its own API that encapsulates enumeration like this one
and this patch would just be reimplementing that API using the list
API, instead of rewriting the users of mru API to directly access
the list API.

Alas, there is no such mru API that lets a mru user to iterate over
elements, so the original of the above code were using mru's
implementation detail directly.

We probably want to invent mru_for_each() that hides the fact that
mru is implemented in terms of list_head from the users of mru API
and use it here.

> diff --git a/mru.c b/mru.c
> index 9dedae0287ed2..8b6ba3d9b7fad 100644
> --- a/mru.c
> +++ b/mru.c
> @@ -1,50 +1,29 @@
>  #include "cache.h"
>  #include "mru.h"
>  
> -void mru_append(struct mru *mru, void *item)
> +void mru_append(struct mru *head, void *item)
>  {
> - struct mru_entry *cur = xmalloc(sizeof(*cur));
> + struct mru *cur = xmalloc(sizeof(*cur));
>   cur->item = item;
> - cur->prev = mru->tail;
> - cur->next = NULL;
> -
> - if (mru->tail)
> - mru->tail->next = cur;
> - else
> - mru->head = cur;
> - mru->tail = cur;
> + list_add_tail(>list, >list);
>  }
>  
> -void mru_mark(struct mru *mru, struct mru_entry *entry)
> +void mru_mark(struct mru *head, struct mru *entry)
>  {
> - /* If we're already at the front of the list, nothing to do */
> - if (mru->head == entry)
> - return;
> -
> - /* Otherwise, remove us from our current slot... */
> - if (entry->prev)
> - entry->prev->next = entry->next;
> - if (entry->next)
> - entry->next->prev = entry->prev;
> - else
> - mru->tail = entry->prev;
> -
> - /* And insert us at the beginning. */
> - entry->prev = NULL;
> - entry->next = mru->head;
> - if (mru->head)
> - mru->head->prev = entry;
> - mru->head = entry;
> + /* To mark means to put at the front of the list. */
> + list_del(>list);
> + list_add(>list, >list);
>  }
>  
> -void mru_clear(struct mru *mru)
> +void mru_clear(struct mru *head)
>  {
> - struct mru_entry *p = mru->head;
> -
> - while (p) {
> - struct mru_entry *to_free = p;
> - p = p->next;
> + struct list_head *p1;
> + struct list_head *p2;
> + struct mru *to_free;
> + 
> + list_for_each_safe(p1, p2, >list) {
> + to_free = list_entry(p1, struct mru, list);
>   free(to_free);
>   }
> - mru->head = mru->tail = NULL;
> + INIT_LIST_HEAD(>list);
>  }
> diff --git a/mru.h b/mru.h
> index 42e4aeaa1098a..36a332af0bf88 100644
> --- a/mru.h
> +++ b/mru.h
> @@ -1,6 +1,8 @@
>  #ifndef MRU_H
>  #define MRU_H
>  
> +#include "list.h"
> +
>  /**
>   * A simple most-recently-used cache, backed by a doubly-linked list.
>   *
> @@ -8,18 +10,15 @@
>   *
>   *   // Create a list.  Zero-initialization is required.
>   

Re: [PATCH 1/1] fast-import: checkpoint: dump branches/tags/marks even if object_count==0

[Junio C Hamano]

"Eric Rannaud"  writes:

> +# The commands in input_file should not produce any output on the file
> +# descriptor set with --cat-blob-fd (or stdout if unspecified).

Thanks for documenting this.  Swapping the order of starting
fast-import and feeding its input (which is one change in this
version relative to the previous one) alone would not help, because
in the updated order in this patch, nobody is reading from
fast-import until the parent process finishes feeding it.

[PATCH Outreachy] mru: use double-linked list from list.h

[Olga Telezhnaya]

Simplify mru.c, mru.h and related code by reusing the double-linked list 
implementation from list.h instead of a custom one.

Signed-off-by: Olga Telezhnaia 
Mentored-by: Christian Couder 
Mentored by: Jeff King 
---
 builtin/pack-objects.c |  5 +++--
 mru.c  | 51 +++---
 mru.h  | 31 +-
 packfile.c |  6 --
 4 files changed, 35 insertions(+), 58 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index f721137eaf881..ba812349e0aab 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -995,8 +995,8 @@ static int want_object_in_pack(const unsigned char *sha1,
   struct packed_git **found_pack,
   off_t *found_offset)
 {
-   struct mru_entry *entry;
int want;
+   struct list_head *pos;
 
if (!exclude && local && has_loose_object_nonlocal(sha1))
return 0;
@@ -1012,7 +1012,8 @@ static int want_object_in_pack(const unsigned char *sha1,
return want;
}
 
-   for (entry = packed_git_mru.head; entry; entry = entry->next) {
+   list_for_each(pos, _git_mru.list) {
+   struct mru *entry = list_entry(pos, struct mru, list);
struct packed_git *p = entry->item;
off_t offset;
 
diff --git a/mru.c b/mru.c
index 9dedae0287ed2..8b6ba3d9b7fad 100644
--- a/mru.c
+++ b/mru.c
@@ -1,50 +1,29 @@
 #include "cache.h"
 #include "mru.h"
 
-void mru_append(struct mru *mru, void *item)
+void mru_append(struct mru *head, void *item)
 {
-   struct mru_entry *cur = xmalloc(sizeof(*cur));
+   struct mru *cur = xmalloc(sizeof(*cur));
cur->item = item;
-   cur->prev = mru->tail;
-   cur->next = NULL;
-
-   if (mru->tail)
-   mru->tail->next = cur;
-   else
-   mru->head = cur;
-   mru->tail = cur;
+   list_add_tail(>list, >list);
 }
 
-void mru_mark(struct mru *mru, struct mru_entry *entry)
+void mru_mark(struct mru *head, struct mru *entry)
 {
-   /* If we're already at the front of the list, nothing to do */
-   if (mru->head == entry)
-   return;
-
-   /* Otherwise, remove us from our current slot... */
-   if (entry->prev)
-   entry->prev->next = entry->next;
-   if (entry->next)
-   entry->next->prev = entry->prev;
-   else
-   mru->tail = entry->prev;
-
-   /* And insert us at the beginning. */
-   entry->prev = NULL;
-   entry->next = mru->head;
-   if (mru->head)
-   mru->head->prev = entry;
-   mru->head = entry;
+   /* To mark means to put at the front of the list. */
+   list_del(>list);
+   list_add(>list, >list);
 }
 
-void mru_clear(struct mru *mru)
+void mru_clear(struct mru *head)
 {
-   struct mru_entry *p = mru->head;
-
-   while (p) {
-   struct mru_entry *to_free = p;
-   p = p->next;
+   struct list_head *p1;
+   struct list_head *p2;
+   struct mru *to_free;
+   
+   list_for_each_safe(p1, p2, >list) {
+   to_free = list_entry(p1, struct mru, list);
free(to_free);
}
-   mru->head = mru->tail = NULL;
+   INIT_LIST_HEAD(>list);
 }
diff --git a/mru.h b/mru.h
index 42e4aeaa1098a..36a332af0bf88 100644
--- a/mru.h
+++ b/mru.h
@@ -1,6 +1,8 @@
 #ifndef MRU_H
 #define MRU_H
 
+#include "list.h"
+
 /**
  * A simple most-recently-used cache, backed by a doubly-linked list.
  *
@@ -8,18 +10,15 @@
  *
  *   // Create a list.  Zero-initialization is required.
  *   static struct mru cache;
- *   mru_append(, item);
- *   ...
+ *   INIT_LIST_HEAD();
  *
- *   // Iterate in MRU order.
- *   struct mru_entry *p;
- *   for (p = cache.head; p; p = p->next) {
- * if (matches(p->item))
- * break;
- *   }
+ *   // Add new item to the end of the list.
+ *   void *item;
+ *   ...
+ *   mru_append(, item);
  *
  *   // Mark an item as used, moving it to the front of the list.
- *   mru_mark(, p);
+ *   mru_mark(, item);
  *
  *   // Reset the list to empty, cleaning up all resources.
  *   mru_clear();
@@ -29,17 +28,13 @@
  * you will begin traversing the whole list again.
  */
 
-struct mru_entry {
-   void *item;
-   struct mru_entry *prev, *next;
-};
-
 struct mru {
-   struct mru_entry *head, *tail;
+   struct list_head list;
+void *item;
 };
 
-void mru_append(struct mru *mru, void *item);
-void mru_mark(struct mru *mru, struct mru_entry *entry);
-void mru_clear(struct mru *mru);
+void mru_append(struct mru *head, void *item);
+void mru_mark(struct mru *head, struct mru *entry);
+void mru_clear(struct mru *head);
 
 #endif /* MRU_H */
diff --git a/packfile.c b/packfile.c
index f69a5c8d607af..ae3b0b2e9c09a 100644
--- a/packfile.c
+++ 

RE: Business Opportunity

[Cooper, Kathy]

From: Cooper, Kathy
Sent: Thursday, September 28, 2017 3:14 AM
Subject: Business Opportunity


Hello friend,
I have a business opportunity that will benefit us millions of United State 
Dollars. If you are interested, Please reply-to my personal email: 
> for details.

Regards,
Khalid Buhazza
Gm Marketing (Bahrain Petroleum Company)

Disclaimer:
The HIPAA Final Privacy Rule requires covered entities to safeguard
certain Protected Health Information (PHI) related to a person's
healthcare. Information being faxed to you may include PHI after
appropriate authorization from the patient or under circumstances
that do not require patient authorization. You, the recipient, are
obligated to maintain PHI in a safe and secure manner. You may not
re-disclose without additional patient consent or as required by
law. Unauthorized re-disclosure or failure to safeguard PHI could
subject you to penalties described in federal (HIPAA) and state
law. If you the reader of this message are not the intended
recipient, or the employee or agent responsible to deliver it to
the intended recipient, please notify us immediately and destroy
the related message.

Re: [PATCH] fast-import: checkpoint: dump branches/tags/marks even if object_count==0

[Eric Rannaud]

On Wed, Sep 27, 2017 at 11:02 PM, Junio C Hamano  wrote:
> Eric Rannaud  writes:
>
>> Doesn't fast-import get a copy of 8 (open for both reading and
>> writing), as a child process, and exec 8>&- only closes the copy of
>> the file descriptor in the parent shell, so the named pipe remains
>> open for writing somewhere (in the fast-import process itself, in
>> fact), therefore fast-import will not find EOF on its stdin?
>
> A.  If that was done intentionally, well, I really have to
> marvel at the cleverness of the solution!  It makes sense now to me.

I plead the Fifth.

Re: [PATCH] fast-import: checkpoint: dump branches/tags/marks even if object_count==0

[Junio C Hamano]

Eric Rannaud  writes:

> Doesn't fast-import get a copy of 8 (open for both reading and
> writing), as a child process, and exec 8>&- only closes the copy of
> the file descriptor in the parent shell, so the named pipe remains
> open for writing somewhere (in the fast-import process itself, in
> fact), therefore fast-import will not find EOF on its stdin?

A.  If that was done intentionally, well, I really have to
marvel at the cleverness of the solution!  It makes sense now to me.