Re: [PATCH v10 12/12] bisect--helper: `get_terms` & `bisect_terms` shell function in C

[Pranit Bauva]

Hey Junio,

On Tue, Jul 26, 2016 at 11:02 PM, Junio C Hamano  wrote:
> Torsten Bögershausen  writes:
>
>> On 07/25/2016 06:53 PM, Junio C Hamano wrote:
>>> Pranit Bauva  writes:
>>>
>> >>> +enum terms_defined {
>> >>> +   TERM_BAD = 1,
>> >>> +   TERM_GOOD = 2,
>> >>> +   TERM_NEW = 4,
>> >>> +   TERM_OLD = 8
>> >>> +};
>> >>> +
> >> ...
>> Is there any risk that a more generic term like "TERM_BAD" may collide
>> with some other definition some day ?
>>
>> Would it make sense to call it GIT_BISECT_TERM_BAD, GBS_TERM_BAD,
>> BIS_TERM_BAD or something more unique ?
>
> I am not sure if the scope of these symbols would ever escape
> outside bisect-helper.c (and builtin/bisect.c eventually when we
> retire git-bisect.sh), but BISECT_TERM_{GOOD,BAD,OLD,NEW} would not
> be too bad.

I agree that it wouldn't be too bad. This can be considered as low
hanging fruit and picked up after the completion of the project as
after the whole conversion, some re-ordering of code would need to be
done. For eg. there is read_bisect_terms() is in bisect.c while
get_terms() is in builtin/bisect--helper.c but they both do the same
stuff except the later one uses strbuf and a lot more important stuff.
Maybe after the whole conversion, the above enum (or #define bitmap)
should also be moved to bisect.h and be used consistently in bisect.c
too.

Regards,
Pranit Bauva
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/4] submodule: support running in multiple worktree setup

[Max Kirillov]

Hi.

On Wed, Jul 20, 2016 at 07:24:18PM +0200, Nguyễn Thái Ngọc Duy wrote:
> + - `remote.*` added by submodules may be per working directory as
> +   well, unless you are sure remotes from all possible submodules in
> +   history are consistent.
...
> @@ -1114,7 +1114,7 @@ cmd_sync()
>   sanitize_submodule_env
>   cd "$sm_path"
>   remote=$(get_default_remote)
> - git config remote."$remote".url 
> "$sub_origin_url"
> + git config --worktree remote."$remote".url 
> "$sub_origin_url"
>  
>   if test -n "$recursive"
>   then

I don't think remote.* should be per-worktree. 

* note that it is sumodule repository, not superproject. It
  does not even have to have multiple worktrees.
* it is quite bad to have it different in worktree, because
  git fetch then results in different ref updates depending
  on where it was called. So whatever issue it was intended
  to solve, it hardly made things better.
* I'm not sure I know all use cases of "submodule sync",
  but as far as I understand, it should be called when the
  submodule repository stays the "same" (however user
  defines the "same"), but older url does not work for some
  reason. Then I think it is correct to change the remote
  url for all worktrees.

-- 
Max
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/5] convert: generate large test files only once

[Torsten Bögershausen]

On 07/27/2016 02:06 AM, larsxschnei...@gmail.com wrote:

From: Lars Schneider 

Generate a more interesting large test file with random characters in
between and reuse this test file in multiple tests. Run tests formerly
marked as EXPENSIVE every time but with a smaller test file.

Signed-off-by: Lars Schneider 
---
 t/t0021-conversion.sh | 35 +--
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 7b45136..b9911a4 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -4,6 +4,13 @@ test_description='blob conversion via gitattributes'

 . ./test-lib.sh

+if test_have_prereq EXPENSIVE
+then
+   T0021_LARGE_FILE_SIZE=2048
+else
+   T0021_LARGE_FILE_SIZE=30
+fi
+
 cat test.i &&
git add test test.t test.i &&
rm -f test test.t test.i &&
-   git checkout -- test test.t test.i
+   git checkout -- test test.t test.i &&
+
+   mkdir -p generated-test-data &&
+   for i in $(test_seq 1 $T0021_LARGE_FILE_SIZE)
+   do
+   # Generate 1MB of empty data and 100 bytes of random characters
+   printf "%1048576d" 1
+   printf "$(LC_ALL=C tr -dc "A-Za-z0-9" >8)) count=1 2>/dev/null)"

I'm not sure how portable /dev/urandom is.
The other thing, that "really random" numbers are an overkill, and
it may be easier to use pre-defined numbers,

The rest of 1..4 looks good, I will look at 5/5 later.


+   done >generated-test-data/large.file
 '

 script='s/^\$Id: \([0-9a-f]*\) \$/\1/p'
@@ -199,9 +214,9 @@ test_expect_success 'required filter clean failure' '
 test_expect_success 'filtering large input to small output should use little 
memory' '
test_config filter.devnull.clean "cat >/dev/null" &&
test_config filter.devnull.required true &&
-   for i in $(test_seq 1 30); do printf "%1048576d" 1; done >30MB &&
-   echo "30MB filter=devnull" >.gitattributes &&
-   GIT_MMAP_LIMIT=1m GIT_ALLOC_LIMIT=1m git add 30MB
+   cp generated-test-data/large.file large.file &&
+   echo "large.file filter=devnull" >.gitattributes &&
+   GIT_MMAP_LIMIT=1m GIT_ALLOC_LIMIT=1m git add large.file
 '

 test_expect_success 'filter that does not read is fine' '
@@ -214,15 +229,15 @@ test_expect_success 'filter that does not read is fine' '
test_cmp expect actual
 '

-test_expect_success EXPENSIVE 'filter large file' '
+test_expect_success 'filter large file' '
test_config filter.largefile.smudge cat &&
test_config filter.largefile.clean cat &&
-   for i in $(test_seq 1 2048); do printf "%1048576d" 1; done >2GB &&
-   echo "2GB filter=largefile" >.gitattributes &&
-   git add 2GB 2>err &&
+   echo "large.file filter=largefile" >.gitattributes &&
+   cp generated-test-data/large.file large.file &&
+   git add large.file 2>err &&
test_must_be_empty err &&
-   rm -f 2GB &&
-   git checkout -- 2GB 2>err &&
+   rm -f large.file &&
+   git checkout -- large.file 2>err &&
test_must_be_empty err
 '



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/5] convert: add filter..process option

[Jeff King]

On Wed, Jul 27, 2016 at 02:06:05AM +0200, larsxschnei...@gmail.com wrote:

> +static off_t multi_packet_read(struct strbuf *sb, const int fd, const size_t 
> size)
> +{
> + off_t bytes_read;
> + off_t total_bytes_read = 0;

I haven't looked carefully at the whole patch yet, but there seems to be
some type issues here. off_t is a good type for storing the whole size
of a file (which may be larger than the amount of memory we can
allocate). But size_t is the right size for an in-memory object.

This function takes a size_t size, which makes sense if it is meant to
read everything into a strbuf.

So I think our total_bytes_read would probably want to be a size_t here,
too, because it cannot possibly grow larger than that (and that is
enforced by the loop below). Otherwise you get weirdness like "sb->buf +
total_bytes_ref" possibly overflowing memory.

> + strbuf_grow(sb, size + 1);  // we need one extra byte for the 
> packet flush

What happens if size is the maximum for size_t here (i.e., 4GB-1 on a
32-bit system)?

> + do {
> + bytes_read = packet_read(
> + fd, NULL, NULL,
> + sb->buf + total_bytes_read, sb->len - total_bytes_read 
> - 1,
> + PACKET_READ_GENTLE_ON_EOF
> + );

packet_read() actually returns an int, and may return "-1" on EOF (and
int is fine because we know that we are constrained to 16-bit values
by the pkt-line definition). You read it into an "off_t". I _think_ that
is OK, because I believe POSIX says off_t must be signed. But probably
"int" is the more correct type here.

> + total_bytes_read += bytes_read;

If you do get "-1", I think you need to detect it here before adjusting
total_bytes_read.

> + while (
> + bytes_read > 0 &&   // the 
> last packet was no flush
> + sb->len - total_bytes_read - 1 > 0  // we still have space 
> left in the buffer
> + );

And I'm not sure if you need to distinguish between "0" and "-1" when
checking byte_read here.

> + strbuf_setlen(sb, total_bytes_read);

Passing an off_t to something expecting a size_t, which can involve
truncation (though I think in practice you really are limited to
size_t).

> +static int multi_packet_write(const char *src, size_t len, const int in, 
> const int out)
> +{
> + int ret = 1;
> + char header[4];
> + char buffer[8192];
> + off_t bytes_to_write;
> + while (ret) {
> + if (in >= 0) {
> + bytes_to_write = xread(in, buffer, sizeof(buffer));

Likewise here, xread() is returning ssize_t. Again, OK if we can assume
off_t is signed, but it probably makes sense to use the correct type (we
also know it cannot be larger than 8K, of course).

Why 8K? The pkt-line format naturally restricts us to just under 64K, so
why not take advantage of that and minimize the framing overhead for
large data?

> + if (bytes_to_write < 0)
> + ret &= 0;

I think "&= 0" is unusual for our codebase? Would just writing "= 0" be
more clear?

We do sometimes do "ret |= something()" but that is in cases where
"ret" is zero for success, and non-zero (usually -1) otherwise. Perhaps
your function's error-reporting is inverted from our usual style?

> + set_packet_header(header, bytes_to_write + 4);
> + ret &= write_in_full(out, , sizeof(header)) == 
> sizeof(header);
> + ret &= write_in_full(out, src, bytes_to_write) == 
> bytes_to_write;
> + }

If you look at format_packet(), it pulls a slight trick: we have a
buffer 4 bytes larger than we need, format into "buf + 4", and then
write the final size at the beginning. That lets us write() it all in
one go.

At first I thought this function was simply reinventing packet_write(),
but I guess you are trying to avoid the extra copy of the data (once
into the buffer from xread, and then again via format_packet just to add
the extra bytes at the beginning).

I agree with what Junio said elsewhere, that there may be a way to make
the pkt-line code handle this zero-copy situation better. Perhaps
something like:

  struct pktline {
/* first 4 bytes are reserved for length header */
char buf[LARGE_PACKET_MAX];
  };
  #define PKTLINE_DATA_START(pkt) ((pkt)->buf + 4)
  #define PKTLINE_DATA_LEN (LARGE_PACKET_MAX - 4)

  ...
  struct pktline pkt;
  ssize_t len = xread(fd, PKTLINE_DATA_START(), PKTLINE_DATA_LEN);
  packet_send(, len);

Then packet_send() knows that the first 4 bytes are reserved for it. I
suspect that the strbuf used by format_packet() could get away with
using such a "struct pktline" too, though in practice I doubt there's
any real efficiency to be gained (we generally reuse the same strbuf
over and over, so it will grow once to 64K and get reused).

> + ret &= write_in_full(out, "", 4) == 4;

packet_flush() ?

I know the packet functions are keen on 

[PATCH v2 3/3] subtree: adjust style to match CodingGuidelines

[David Aguilar]

Prefer "test" over "[ ... ]", use double-quotes around variables, break
long lines, and properly indent "case" statements.

Helped-by: Johannes Sixt 
Signed-off-by: David Aguilar 
---
This is a replacement patch that addresses the notes from Hannes' review.

 contrib/subtree/git-subtree.sh | 570 +
 1 file changed, 354 insertions(+), 216 deletions(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index b567eae..9fd3b6a 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -4,8 +4,9 @@
 #
 # Copyright (C) 2009 Avery Pennarun 
 #
-if [ $# -eq 0 ]; then
-set -- -h
+if test $# -eq 0
+then
+   set -- -h
 fi
 OPTS_SPEC="\
 git subtree add   --prefix= 
@@ -50,87 +51,145 @@ prefix=
 
 debug()
 {
-   if [ -n "$debug" ]; then
+   if test -n "$debug"
+   then
printf "%s\n" "$*" >&2
fi
 }
 
 say()
 {
-   if [ -z "$quiet" ]; then
+   if test -z "$quiet"
+   then
printf "%s\n" "$*" >&2
fi
 }
 
 progress()
 {
-   if [ -z "$quiet" ]; then
+   if test -z "$quiet"
+   then
printf "%s\r" "$*" >&2
fi
 }
 
 assert()
 {
-   if "$@"; then
-   :
-   else
+   if ! "$@"
+   then
die "assertion failed: " "$@"
fi
 }
 
 
-#echo "Options: $*"
-
-while [ $# -gt 0 ]; do
+while test $# -gt 0
+do
opt="$1"
shift
+
case "$opt" in
-   -q) quiet=1 ;;
-   -d) debug=1 ;;
-   --annotate) annotate="$1"; shift ;;
-   --no-annotate) annotate= ;;
-   -b) branch="$1"; shift ;;
-   -P) prefix="${1%/}"; shift ;;
-   -m) message="$1"; shift ;;
-   --no-prefix) prefix= ;;
-   --onto) onto="$1"; shift ;;
-   --no-onto) onto= ;;
-   --rejoin) rejoin=1 ;;
-   --no-rejoin) rejoin= ;;
-   --ignore-joins) ignore_joins=1 ;;
-   --no-ignore-joins) ignore_joins= ;;
-   --squash) squash=1 ;;
-   --no-squash) squash= ;;
-   --) break ;;
-   *) die "Unexpected option: $opt" ;;
+   -q)
+   quiet=1
+   ;;
+   -d)
+   debug=1
+   ;;
+   --annotate)
+   annotate="$1"
+   shift
+   ;;
+   --no-annotate)
+   annotate=
+   ;;
+   -b)
+   branch="$1"
+   shift
+   ;;
+   -P)
+   prefix="${1%/}"
+   shift
+   ;;
+   -m)
+   message="$1"
+   shift
+   ;;
+   --no-prefix)
+   prefix=
+   ;;
+   --onto)
+   onto="$1"
+   shift
+   ;;
+   --no-onto)
+   onto=
+   ;;
+   --rejoin)
+   rejoin=1
+   ;;
+   --no-rejoin)
+   rejoin=
+   ;;
+   --ignore-joins)
+   ignore_joins=1
+   ;;
+   --no-ignore-joins)
+   ignore_joins=
+   ;;
+   --squash)
+   squash=1
+   ;;
+   --no-squash)
+   squash=
+   ;;
+   --)
+   break
+   ;;
+   *)
+   die "Unexpected option: $opt"
+   ;;
esac
 done
 
 command="$1"
 shift
+
 case "$command" in
-   add|merge|pull) default= ;;
-   split|push) default="--default HEAD" ;;
-   *) die "Unknown command '$command'" ;;
+add|merge|pull)
+   default=
+   ;;
+split|push)
+   default="--default HEAD"
+   ;;
+*)
+   die "Unknown command '$command'"
+   ;;
 esac
 
-if [ -z "$prefix" ]; then
+if test -z "$prefix"
+then
die "You must provide the --prefix option."
 fi
 
 case "$command" in
-   add) [ -e "$prefix" ] && 
-   die "prefix '$prefix' already exists." ;;
-   *)   [ -e "$prefix" ] || 
-   die "'$prefix' does not exist; use 'git subtree add'" ;;
+add)
+   test -e "$prefix" && die "prefix '$prefix' already exists."
+   ;;
+*)
+   test -e "$prefix" ||
+   die "'$prefix' does not exist; use 'git subtree add'"
+   ;;
 esac
 
 dir="$(dirname "$prefix/.")"
 
-if [ "$command" != "pull" -a "$command" != "add" -a "$command" != "push" ]; 
then
+if test "$command" != "pull" &&
+   test "$command" != "add" &&
+   test "$command" != "push"
+then
revs=$(git rev-parse $default --revs-only "$@") || exit $?
-   dirs="$(git rev-parse --no-revs --no-flags "$@")" || exit $?
-   if [ -n "$dirs" ]; then
+   dirs=$(git rev-parse --no-revs --no-flags "$@") || exit $?
+   if test -n "$dirs"
+   then
die "Error: 

Re: [PATCH v2 3/5] pkt-line: extract and use `set_packet_header` function

[Junio C Hamano]

larsxschnei...@gmail.com writes:

> From: Lars Schneider 
>
> `set_packet_header` converts an integer to a 4 byte hex string. Make
> this function publicly available so that other parts of Git can easily
> generate a pkt-line.

I think that having to do this is a strong sign that the design of
this series is going in a wrong direction.

If you need a helper function that writes a pkt-line format that
behaves differently from what is already available (for example,
packet_write()), it would be much better to design that new function
so that it would be generally useful and add that to pkt-line.[ch],
instead of creating random helper functions that use write(2)
directly, bypassing pkt-line API, to write stuff.

In other words, do not _mimick_ pkt-line; enhance pkt-line as
necessary and use it.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 0/5] Git filter protocol

[larsxschneider]

From: Lars Schneider 

Hi,

thanks a lot for the extensive reviews. I tried to address all mentioned
concerns and summarized them below. The most prominent changes since v1 are
the following:
* pipe communication uses a packet format (pkt-line) based protocol
* a long running filter application is defined with "filter..process"
* Git offers a number of filter capabilties that a filter can request
  (right now only "smudge" and "clean" - in the future maybe "cleanFromFile",
  "smudgeToFile", and/or "stream")

Cheers,
Lars


## Torsten:
* add "\n" line terminator after version in init sequence
* prepare big file for EXPENSIVE tests once
* set "#!/usr/bin/perl" as shebang for rot13.pl to mimic other Perl test scripts
* add test_have_prereq PERL to t0021

## Ramsay:
* use write_in_full(process->in, nbuf.buf, nbuf.len) to avoid unneccsary strlen 
call
* use read_in_full to read data that exceeds MAX_IO_SIZE properly
* fix test case to check for large file filtering

## Jakub:
* use standard input/standard output instead of stdin/stdout
* replace global variable "cmd_process_map" with a function parameter where 
possible
* avoid "strbuf_reset" after STRBUF_INIT
* align test_config_global
* rename rot13.pl to rot13-filter.pl
* make Perl style consistent
* describe hard coded filenames in test filter header
* improve docs
* add filter capabilities field (enables cleanToFile, smudgeFromFile, and/or 
stream later)
* explain that content size in bytes is encoded in ASCII
* consistent line ending for die call in Perl (without "\n")
* make rot13 test filter die in case of failure (instead of returning "fail")

## Eric:
* flush explicitly in Perl test filter
* do not initialize variables to NULL if they are set unconditionally
* fix no-op stop_protocol_filter
* use off_t instead of size_t
* improve test filter int parsing ($filelen =~ /\A\d+\z/ or die "bad filelen: 
$filelen")

## Peff:
* use pkt-line protocol
* do not use Perl autodie

## Remi:
* remove spaces after '<'



Lars Schneider (5):
  convert: quote filter names in error messages
  convert: modernize tests
  pkt-line: extract and use `set_packet_header` function
  convert: generate large test files only once
  convert: add filter..process option

 Documentation/gitattributes.txt |  54 +++-
 convert.c   | 281 +---
 pkt-line.c  |  15 ++-
 pkt-line.h  |   1 +
 t/t0021-conversion.sh   | 272 --
 t/t0021/rot13-filter.pl | 146 +
 6 files changed, 704 insertions(+), 65 deletions(-)
 create mode 100755 t/t0021/rot13-filter.pl

--
2.9.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/5] convert: modernize tests

[larsxschneider]

From: Lars Schneider 

Use `test_config` to set the config, check that files are empty with
`test_must_be_empty`, compare files with `test_cmp`, and remove spaces
after ">".

Signed-off-by: Lars Schneider 
---
 t/t0021-conversion.sh | 62 +--
 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 7bac2bc..7b45136 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -13,8 +13,8 @@ EOF
 chmod +x rot13.sh
 
 test_expect_success setup '
-   git config filter.rot13.smudge ./rot13.sh &&
-   git config filter.rot13.clean ./rot13.sh &&
+   test_config filter.rot13.smudge ./rot13.sh &&
+   test_config filter.rot13.clean ./rot13.sh &&
 
{
echo "*.t filter=rot13"
@@ -38,8 +38,8 @@ script='s/^\$Id: \([0-9a-f]*\) \$/\1/p'
 
 test_expect_success check '
 
-   cmp test.o test &&
-   cmp test.o test.t &&
+   test_cmp test.o test &&
+   test_cmp test.o test.t &&
 
# ident should be stripped in the repository
git diff --raw --exit-code :test :test.i &&
@@ -47,10 +47,10 @@ test_expect_success check '
embedded=$(sed -ne "$script" test.i) &&
test "z$id" = "z$embedded" &&
 
-   git cat-file blob :test.t > test.r &&
+   git cat-file blob :test.t >test.r &&
 
-   ./rot13.sh < test.o > test.t &&
-   cmp test.r test.t
+   ./rot13.sh test.t &&
+   test_cmp test.r test.t
 '
 
 # If an expanded ident ever gets into the repository, we want to make sure that
@@ -130,7 +130,7 @@ test_expect_success 'filter shell-escaped filenames' '
 
# delete the files and check them out again, using a smudge filter
# that will count the args and echo the command-line back to us
-   git config filter.argc.smudge "sh ./argc.sh %f" &&
+   test_config filter.argc.smudge "sh ./argc.sh %f" &&
rm "$normal" "$special" &&
git checkout -- "$normal" "$special" &&
 
@@ -141,7 +141,7 @@ test_expect_success 'filter shell-escaped filenames' '
test_cmp expect "$special" &&
 
# do the same thing, but with more args in the filter expression
-   git config filter.argc.smudge "sh ./argc.sh %f --my-extra-arg" &&
+   test_config filter.argc.smudge "sh ./argc.sh %f --my-extra-arg" &&
rm "$normal" "$special" &&
git checkout -- "$normal" "$special" &&
 
@@ -154,9 +154,9 @@ test_expect_success 'filter shell-escaped filenames' '
 '
 
 test_expect_success 'required filter should filter data' '
-   git config filter.required.smudge ./rot13.sh &&
-   git config filter.required.clean ./rot13.sh &&
-   git config filter.required.required true &&
+   test_config filter.required.smudge ./rot13.sh &&
+   test_config filter.required.clean ./rot13.sh &&
+   test_config filter.required.required true &&
 
echo "*.r filter=required" >.gitattributes &&
 
@@ -165,17 +165,17 @@ test_expect_success 'required filter should filter data' '
 
rm -f test.r &&
git checkout -- test.r &&
-   cmp test.o test.r &&
+   test_cmp test.o test.r &&
 
./rot13.sh expected &&
git cat-file blob :test.r >actual &&
-   cmp expected actual
+   test_cmp expected actual
 '
 
 test_expect_success 'required filter smudge failure' '
-   git config filter.failsmudge.smudge false &&
-   git config filter.failsmudge.clean cat &&
-   git config filter.failsmudge.required true &&
+   test_config filter.failsmudge.smudge false &&
+   test_config filter.failsmudge.clean cat &&
+   test_config filter.failsmudge.required true &&
 
echo "*.fs filter=failsmudge" >.gitattributes &&
 
@@ -186,9 +186,9 @@ test_expect_success 'required filter smudge failure' '
 '
 
 test_expect_success 'required filter clean failure' '
-   git config filter.failclean.smudge cat &&
-   git config filter.failclean.clean false &&
-   git config filter.failclean.required true &&
+   test_config filter.failclean.smudge cat &&
+   test_config filter.failclean.clean false &&
+   test_config filter.failclean.required true &&
 
echo "*.fc filter=failclean" >.gitattributes &&
 
@@ -197,8 +197,8 @@ test_expect_success 'required filter clean failure' '
 '
 
 test_expect_success 'filtering large input to small output should use little 
memory' '
-   git config filter.devnull.clean "cat >/dev/null" &&
-   git config filter.devnull.required true &&
+   test_config filter.devnull.clean "cat >/dev/null" &&
+   test_config filter.devnull.required true &&
for i in $(test_seq 1 30); do printf "%1048576d" 1; done >30MB &&
echo "30MB filter=devnull" >.gitattributes &&
GIT_MMAP_LIMIT=1m GIT_ALLOC_LIMIT=1m git add 30MB
@@ -207,7 +207,7 @@ test_expect_success 'filtering large input to small output 
should use little mem
 test_expect_success 

[PATCH v2 1/5] convert: quote filter names in error messages

[larsxschneider]

From: Lars Schneider 

Git filter with spaces (e.g. `filter.sh foo`) are hard to read in
error messages. Quote them to improve the readability.

Signed-off-by: Lars Schneider 
---
 convert.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/convert.c b/convert.c
index b1614bf..522e2c5 100644
--- a/convert.c
+++ b/convert.c
@@ -397,7 +397,7 @@ static int filter_buffer_or_fd(int in, int out, void *data)
child_process.out = out;
 
if (start_command(_process))
-   return error("cannot fork to run external filter %s", 
params->cmd);
+   return error("cannot fork to run external filter '%s'", 
params->cmd);
 
sigchain_push(SIGPIPE, SIG_IGN);
 
@@ -415,13 +415,13 @@ static int filter_buffer_or_fd(int in, int out, void 
*data)
if (close(child_process.in))
write_err = 1;
if (write_err)
-   error("cannot feed the input to external filter %s", 
params->cmd);
+   error("cannot feed the input to external filter '%s'", 
params->cmd);
 
sigchain_pop(SIGPIPE);
 
status = finish_command(_process);
if (status)
-   error("external filter %s failed %d", params->cmd, status);
+   error("external filter '%s' failed %d", params->cmd, status);
 
strbuf_release();
return (write_err || status);
@@ -462,15 +462,15 @@ static int apply_filter(const char *path, const char 
*src, size_t len, int fd,
return 0;   /* error was already reported */
 
if (strbuf_read(, async.out, len) < 0) {
-   error("read from external filter %s failed", cmd);
+   error("read from external filter '%s' failed", cmd);
ret = 0;
}
if (close(async.out)) {
-   error("read from external filter %s failed", cmd);
+   error("read from external filter '%s' failed", cmd);
ret = 0;
}
if (finish_async()) {
-   error("external filter %s failed", cmd);
+   error("external filter '%s' failed", cmd);
ret = 0;
}
 
-- 
2.9.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 4/5] convert: generate large test files only once

[larsxschneider]

From: Lars Schneider 

Generate a more interesting large test file with random characters in
between and reuse this test file in multiple tests. Run tests formerly
marked as EXPENSIVE every time but with a smaller test file.

Signed-off-by: Lars Schneider 
---
 t/t0021-conversion.sh | 35 +--
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 7b45136..b9911a4 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -4,6 +4,13 @@ test_description='blob conversion via gitattributes'
 
 . ./test-lib.sh
 
+if test_have_prereq EXPENSIVE
+then
+   T0021_LARGE_FILE_SIZE=2048
+else
+   T0021_LARGE_FILE_SIZE=30
+fi
+
 cat test.i &&
git add test test.t test.i &&
rm -f test test.t test.i &&
-   git checkout -- test test.t test.i
+   git checkout -- test test.t test.i &&
+
+   mkdir -p generated-test-data &&
+   for i in $(test_seq 1 $T0021_LARGE_FILE_SIZE)
+   do
+   # Generate 1MB of empty data and 100 bytes of random characters
+   printf "%1048576d" 1
+   printf "$(LC_ALL=C tr -dc "A-Za-z0-9" >8)) count=1 2>/dev/null)"
+   done >generated-test-data/large.file
 '
 
 script='s/^\$Id: \([0-9a-f]*\) \$/\1/p'
@@ -199,9 +214,9 @@ test_expect_success 'required filter clean failure' '
 test_expect_success 'filtering large input to small output should use little 
memory' '
test_config filter.devnull.clean "cat >/dev/null" &&
test_config filter.devnull.required true &&
-   for i in $(test_seq 1 30); do printf "%1048576d" 1; done >30MB &&
-   echo "30MB filter=devnull" >.gitattributes &&
-   GIT_MMAP_LIMIT=1m GIT_ALLOC_LIMIT=1m git add 30MB
+   cp generated-test-data/large.file large.file &&
+   echo "large.file filter=devnull" >.gitattributes &&
+   GIT_MMAP_LIMIT=1m GIT_ALLOC_LIMIT=1m git add large.file
 '
 
 test_expect_success 'filter that does not read is fine' '
@@ -214,15 +229,15 @@ test_expect_success 'filter that does not read is fine' '
test_cmp expect actual
 '
 
-test_expect_success EXPENSIVE 'filter large file' '
+test_expect_success 'filter large file' '
test_config filter.largefile.smudge cat &&
test_config filter.largefile.clean cat &&
-   for i in $(test_seq 1 2048); do printf "%1048576d" 1; done >2GB &&
-   echo "2GB filter=largefile" >.gitattributes &&
-   git add 2GB 2>err &&
+   echo "large.file filter=largefile" >.gitattributes &&
+   cp generated-test-data/large.file large.file &&
+   git add large.file 2>err &&
test_must_be_empty err &&
-   rm -f 2GB &&
-   git checkout -- 2GB 2>err &&
+   rm -f large.file &&
+   git checkout -- large.file 2>err &&
test_must_be_empty err
 '
 
-- 
2.9.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 5/5] convert: add filter..process option

[larsxschneider]

From: Lars Schneider 

Git's clean/smudge mechanism invokes an external filter process for every
single blob that is affected by a filter. If Git filters a lot of blobs
then the startup time of the external filter processes can become a
significant part of the overall Git execution time.

This patch adds the filter..process string option which, if used,
keeps the external filter process running and processes all blobs with
the following packet format (pkt-line) based protocol over standard input
and standard output.

Git starts the filter on first usage and expects a welcome
message, protocol version number, and filter capabilities
seperated by spaces:

packet:  git< git-filter-protocol
packet:  git< version 2
packet:  git< clean smudge

Supported filter capabilities are "clean" and "smudge".

Afterwards Git sends a command (e.g. "smudge" or "clean" - based
on the supported capabilities), the filename, the content size as
ASCII number in bytes, and the content in packet format with a
flush packet at the end:

packet:  git> smudge
packet:  git> testfile.dat
packet:  git> 7
packet:  git> CONTENT
packet:  git> 


The filter is expected to respond with the result content size as
ASCII number in bytes and the result content in packet format with
a flush packet at the end:

packet:  git< 57
packet:  git< SMUDGED_CONTENT
packet:  git< 

Please note: In a future version of Git the capability "stream"
might be supported. In that case the content size must not be
part of the filter response.

Afterwards the filter is expected to wait for the next command.

Signed-off-by: Lars Schneider 
Helped-by: Martin-Louis Bright 
Signed-off-by: Lars Schneider 
---
 Documentation/gitattributes.txt |  54 +++-
 convert.c   | 269 ++--
 t/t0021-conversion.sh   | 175 ++
 t/t0021/rot13-filter.pl | 146 ++
 4 files changed, 631 insertions(+), 13 deletions(-)
 create mode 100755 t/t0021/rot13-filter.pl

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 8882a3e..8fb40d2 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -300,7 +300,11 @@ checkout, when the `smudge` command is specified, the 
command is
 fed the blob object from its standard input, and its standard
 output is used to update the worktree file.  Similarly, the
 `clean` command is used to convert the contents of worktree file
-upon checkin.
+upon checkin. By default these commands process only a single
+blob and terminate. If a long running filter process (see section
+below) is used then Git can process all blobs with a single filter
+invocation for the entire life of a single Git command (e.g.
+`git add .`).
 
 One use of the content filtering is to massage the content into a shape
 that is more convenient for the platform, filesystem, and the user to use.
@@ -375,6 +379,54 @@ substitution.  For example:
 
 
 
+Long Running Filter Process
+^^^
+
+If the filter command (string value) is defined via
+filter..process then Git can process all blobs with a
+single filter invocation for the entire life of a single Git
+command by talking with the following packet format (pkt-line)
+based protocol over standard input and standard output.
+
+Git starts the filter on first usage and expects a welcome
+message, protocol version number, and filter capabilities
+seperated by spaces:
+
+packet:  git< git-filter-protocol
+packet:  git< version 2
+packet:  git< clean smudge
+
+Supported filter capabilities are "clean" and "smudge".
+
+Afterwards Git sends a command (e.g. "smudge" or "clean" - based
+on the supported capabilities), the filename, the content size as
+ASCII number in bytes, and the content in packet format with a
+flush packet at the end:
+
+packet:  git> smudge
+packet:  git> testfile.dat
+packet:  git> 7
+packet:  git> CONTENT
+packet:  git> 
+
+
+The filter is expected to respond with the result content size as
+ASCII number in bytes and the result content in packet format with
+a flush packet at the end:
+
+packet:  git< 57
+packet:  git< SMUDGED_CONTENT
+packet:  git< 
+
+Please note: In a future version of Git the capability "stream"
+might be supported. In that case the content size must not be
+part of the filter response.
+
+Afterwards the filter is expected to 

[PATCH v2 3/5] pkt-line: extract and use `set_packet_header` function

[larsxschneider]

From: Lars Schneider 

`set_packet_header` converts an integer to a 4 byte hex string. Make
this function publicly available so that other parts of Git can easily
generate a pkt-line.

Signed-off-by: Lars Schneider 
---
 pkt-line.c | 15 ++-
 pkt-line.h |  1 +
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/pkt-line.c b/pkt-line.c
index 62fdb37..6820224 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -98,9 +98,17 @@ void packet_buf_flush(struct strbuf *buf)
 }
 
 #define hex(a) (hexchar[(a) & 15])
-static void format_packet(struct strbuf *out, const char *fmt, va_list args)
+void set_packet_header(char *buf, const int size)
 {
static char hexchar[] = "0123456789abcdef";
+   buf[0] = hex(size >> 12);
+   buf[1] = hex(size >> 8);
+   buf[2] = hex(size >> 4);
+   buf[3] = hex(size);
+}
+
+static void format_packet(struct strbuf *out, const char *fmt, va_list args)
+{
size_t orig_len, n;
 
orig_len = out->len;
@@ -111,10 +119,7 @@ static void format_packet(struct strbuf *out, const char 
*fmt, va_list args)
if (n > LARGE_PACKET_MAX)
die("protocol error: impossibly long line");
 
-   out->buf[orig_len + 0] = hex(n >> 12);
-   out->buf[orig_len + 1] = hex(n >> 8);
-   out->buf[orig_len + 2] = hex(n >> 4);
-   out->buf[orig_len + 3] = hex(n);
+   set_packet_header(>buf[orig_len], n);
packet_trace(out->buf + orig_len + 4, n - 4, 1);
 }
 
diff --git a/pkt-line.h b/pkt-line.h
index 3cb9d91..925c6d3 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -23,6 +23,7 @@ void packet_flush(int fd);
 void packet_write(int fd, const char *fmt, ...) __attribute__((format (printf, 
2, 3)));
 void packet_buf_flush(struct strbuf *buf);
 void packet_buf_write(struct strbuf *buf, const char *fmt, ...) 
__attribute__((format (printf, 2, 3)));
+void set_packet_header(char *buf, const int size);
 
 /*
  * Read a packetized line into the buffer, which must be at least size bytes
-- 
2.9.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] fix passing a name for config from submodules

[Junio C Hamano]

Stefan Beller  writes:

>> @@ -425,8 +432,9 @@ static const struct submodule *config_from(struct 
>> submodule_cache *cache,
>> parameter.commit_sha1 = commit_sha1;
>> parameter.gitmodules_sha1 = sha1;
>> parameter.overwrite = 0;
>> -   git_config_from_mem(parse_config, "submodule-blob", "",
>> +   git_config_from_mem(parse_config, "submodule-blob", rev.buf,
>> config, config_size, );
>
> Ok, this is the actual fix. Do you want to demonstrate its impact by adding
> one or two tests that failed before and now work?
> (As I was using the submodule config API most of the time with null_sha1
> to indicate we'd be looking at the current .gitmodules file in the worktree,
> the actual bug may have not manifested in the users of this API.
> But still, it would be nice to see what was broken?)

Sounds like a good idea.  I'll keep these two queued on 'pu' and see
if Heiko (or somebody else) can find time to do that, so that we can
replace them with an improved version when it happens.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] pack-objects: break out of want_object loop early

[Junio C Hamano]

Jeff King  writes:

> I got side-tracked by adding a t/perf test to show off the improvement.
> It's rather tricky to get right and takes a long time to run. I _think_
> I have it now, but am waiting for results. :)

Well, then I'd stop here and wait for the reroll to requeue.

Thanks.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] push: add shorthand for --force-with-lease branch creation

[Junio C Hamano]

John Keeping  writes:

> Thanks.  I'm about to send v3 anyway to pull a test forward to address
> Jakub's comment.  I also used oidclr() for the last two changes below.

Will replace with v3.

I think v3 is ready to advance to 'next'.  Let's see if we get
further comments from others for a few days.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 00/41] libify apply and use lib in am, part 2

[Junio C Hamano]

Christian Couder  writes:

> Sorry if this patch series is still long. I can split it into two or
> more series if it is prefered.
> ...
> Christian Couder (41):
>   apply: make some names more specific
>   apply: move 'struct apply_state' to apply.h
>   builtin/apply: make apply_patch() return -1 or -128 instead of
> die()ing
>   builtin/apply: read_patch_file() return -1 instead of die()ing
>   builtin/apply: make find_header() return -128 instead of die()ing
>   builtin/apply: make parse_chunk() return a negative integer on error
>   builtin/apply: make parse_single_patch() return -1 on error
>   builtin/apply: make parse_whitespace_option() return -1 instead of
> die()ing
>   builtin/apply: make parse_ignorewhitespace_option() return -1 instead
> of die()ing
>   builtin/apply: move init_apply_state() to apply.c
>   apply: make init_apply_state() return -1 instead of exit()ing
>   builtin/apply: make check_apply_state() return -1 instead of die()ing
>   builtin/apply: move check_apply_state() to apply.c
>   builtin/apply: make apply_all_patches() return 128 or 1 on error
>   builtin/apply: make parse_traditional_patch() return -1 on error
>   builtin/apply: make gitdiff_*() return 1 at end of header
>   builtin/apply: make gitdiff_*() return -1 on error
>   builtin/apply: change die_on_unsafe_path() to check_unsafe_path()
>   builtin/apply: make build_fake_ancestor() return -1 on error
>   builtin/apply: make remove_file() return -1 on error
>   builtin/apply: make add_conflicted_stages_file() return -1 on error
>   builtin/apply: make add_index_file() return -1 on error
>   builtin/apply: make create_file() return -1 on error
>   builtin/apply: make write_out_one_result() return -1 on error
>   builtin/apply: make write_out_results() return -1 on error
>   builtin/apply: make try_create_file() return -1 on error
>   builtin/apply: make create_one_file() return -1 on error
>   builtin/apply: rename option parsing functions
>   apply: rename and move opt constants to apply.h
>   Move libified code from builtin/apply.c to apply.{c,h}
>   apply: make some parsing functions static again
>   environment: add set_index_file()
>   write_or_die: use warning() instead of fprintf(stderr, ...)
>   apply: add 'be_silent' variable to 'struct apply_state'
>   apply: make 'be_silent' incompatible with 'apply_verbosely'
>   apply: don't print on stdout when be_silent is set
>   usage: add set_warn_routine()
>   usage: add get_error_routine() and get_warn_routine()
>   apply: change error_routine when be_silent is set
>   builtin/am: use apply api in run_apply()
>   apply: use error_errno() where possible

I finally found time to get back to finish reviewing this.

The early part up to and including "apply: make some parsing
functions static again" looked good and we could treat them as a
continuation of the earlier cc/apply-introduce-state topic, which
has been merged to the 'master' already.

I got an impression that the patches in the remainder of the series
were not as polished as the earlier ones, except for the last one,
which should be reordered and be part of the early preparation step
if possible.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 4/8] status: per-file data collection for --porcelain=v2

[Jeff Hostetler]

From: Jeff Hostetler 

The output of `git status --porcelain` leaves out many details
about the current status that clients might like to have.  This
can force them to be less efficient as they may need to launch
secondary commands (and try to match the logic within git) to
accumulate this extra information.  For example, a GUI IDE might
want the file mode to display the correct icon for a changed
item (without having to stat it afterwards).

Signed-off-by: Jeff Hostetler 
Signed-off-by: Jeff Hostetler 
---
 builtin/commit.c |  3 +++
 wt-status.c  | 63 +++-
 wt-status.h  |  4 
 3 files changed, 69 insertions(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index c3ae2c3..93ce28c 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -153,6 +153,8 @@ static int opt_parse_porcelain(const struct option *opt, 
const char *arg, int un
*value = STATUS_FORMAT_PORCELAIN;
else if (!strcmp(arg, "v1") || !strcmp(arg,"1"))
*value = STATUS_FORMAT_PORCELAIN;
+   else if (!strcmp(arg, "v2") || !strcmp(arg, "2"))
+   *value = STATUS_FORMAT_PORCELAIN_V2;
else
die("unsupported porcelain version '%s'", arg);
 
@@ -1104,6 +1106,7 @@ static struct status_deferred_config {
 static void finalize_deferred_config(struct wt_status *s)
 {
int use_deferred_config = (status_format != STATUS_FORMAT_PORCELAIN &&
+  status_format != STATUS_FORMAT_PORCELAIN_V2 
&&
   !s->null_termination);
 
if (s->null_termination) {
diff --git a/wt-status.c b/wt-status.c
index a9031e4..15d3349 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -434,6 +434,31 @@ static void wt_status_collect_changed_cb(struct 
diff_queue_struct *q,
if (S_ISGITLINK(p->two->mode))
d->new_submodule_commits = !!oidcmp(>one->oid,
>two->oid);
+
+   switch (p->status) {
+   case DIFF_STATUS_ADDED:
+   die("BUG: worktree status add???");
+   break;
+
+   case DIFF_STATUS_DELETED:
+   d->mode_index = p->one->mode;
+   oidcpy(>oid_index, >one->oid);
+   /* mode_worktree is zero for a delete. */
+   break;
+
+   case DIFF_STATUS_MODIFIED:
+   case DIFF_STATUS_TYPE_CHANGED:
+   case DIFF_STATUS_UNMERGED:
+   d->mode_index = p->one->mode;
+   d->mode_worktree = p->two->mode;
+   oidcpy(>oid_index, >one->oid);
+   break;
+
+   case DIFF_STATUS_UNKNOWN:
+   die("BUG: worktree status unknown???");
+   break;
+   }
+
}
 }
 
@@ -479,12 +504,36 @@ static void wt_status_collect_updated_cb(struct 
diff_queue_struct *q,
if (!d->index_status)
d->index_status = p->status;
switch (p->status) {
+   case DIFF_STATUS_ADDED:
+   /* Leave {mode,oid}_head zero for an add. */
+   d->mode_index = p->two->mode;
+   oidcpy(>oid_index, >two->oid);
+   break;
+   case DIFF_STATUS_DELETED:
+   d->mode_head = p->one->mode;
+   oidcpy(>oid_head, >one->oid);
+   /* Leave {mode,oid}_index zero for a delete. */
+   break;
+   
case DIFF_STATUS_COPIED:
case DIFF_STATUS_RENAMED:
d->head_path = xstrdup(p->one->path);
+   d->score = p->score * 100 / MAX_SCORE;
+   /* fallthru */
+   case DIFF_STATUS_MODIFIED:
+   case DIFF_STATUS_TYPE_CHANGED:
+   d->mode_head = p->one->mode;
+   d->mode_index = p->two->mode;
+   oidcpy(>oid_head, >one->oid);
+   oidcpy(>oid_index, >two->oid);
break;
case DIFF_STATUS_UNMERGED:
d->stagemask = unmerged_mask(p->two->path);
+   /*
+* Don't bother setting {mode,oid}_{head,index} since 
the print
+* code will output the stage values directly and not 
use the
+* values in these fields.
+*/
break;
}
}
@@ -565,9 +614,18 @@ static void wt_status_collect_changes_initial(struct 
wt_status *s)
if (ce_stage(ce)) {
d->index_status = 

[PATCH v3 5/8] status: print per-file porcelain v2 status data

[Jeff Hostetler]

From: Jeff Hostetler 

Print per-file information in porcelain v2 format.

Signed-off-by: Jeff Hostetler 
Signed-off-by: Jeff Hostetler 
---
 wt-status.c | 283 +++-
 1 file changed, 282 insertions(+), 1 deletion(-)

diff --git a/wt-status.c b/wt-status.c
index 15d3349..46061d4 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1813,6 +1813,287 @@ static void wt_porcelain_print(struct wt_status *s)
wt_shortstatus_print(s);
 }
 
+/*
+ * Convert various submodule status values into a
+ * fixed-length string of characters in the buffer provided.
+ */
+static void wt_porcelain_v2_submodule_state(
+   struct wt_status_change_data *d,
+   char sub[5])
+{
+   if (S_ISGITLINK(d->mode_head) ||
+   S_ISGITLINK(d->mode_index) ||
+   S_ISGITLINK(d->mode_worktree)) {
+   sub[0] = 'S';
+   sub[1] = d->new_submodule_commits ? 'C' : '.';
+   sub[2] = (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED) ? 'M' 
: '.';
+   sub[3] = (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ? 'U' 
: '.';
+   } else {
+   sub[0] = 'N';
+   sub[1] = '.';
+   sub[2] = '.';
+   sub[3] = '.';
+   }
+   sub[4] = 0;
+}
+
+/*
+ * Fix-up changed entries before we print them.
+ */
+static void wt_porcelain_v2_fix_up_changed(
+   struct string_list_item *it,
+   struct wt_status *s)
+{
+   struct wt_status_change_data *d = it->util;
+
+   if (!d->index_status) {
+   /*
+* This entry is unchanged in the index (relative to the head).
+* Therefore, the collect_updated_cb was never called for this
+* entry (during the head-vs-index scan) and so the head column
+* fields were never set.
+*
+* We must have data for the index column (from the
+* index-vs-worktree scan (otherwise, this entry should not be
+* in the list of changes)).
+*
+* Copy index column fields to the head column, so that our
+* output looks complete.
+*/
+   assert(d->mode_head == 0);
+   d->mode_head = d->mode_index;
+   oidcpy(>oid_head, >oid_index);
+   }
+
+   if (!d->worktree_status) {
+   /*
+* This entry is unchanged in the worktree (relative to the 
index).
+* Therefore, the collect_changed_cb was never called for this 
entry
+* (during the index-vs-worktree scan) and so the worktree 
column
+* fields were never set.
+*
+* We must have data for the index column (from the 
head-vs-index
+* scan).
+*
+* Copy the index column fields to the worktree column so that
+* our output looks complete.
+*
+* Note that we only have a mode field in the worktree column
+* because the scan code tries really hard to not have to 
compute it.
+*/
+   assert(d->mode_worktree == 0);
+   d->mode_worktree = d->mode_index;
+   }
+}
+
+/*
+ * Print porcelain v2 info for tracked entries with changes.
+ */
+static void wt_porcelain_v2_print_changed_entry(
+   struct string_list_item *it,
+   struct wt_status *s)
+{
+   struct wt_status_change_data *d = it->util;
+   struct strbuf buf_current = STRBUF_INIT;
+   struct strbuf buf_src = STRBUF_INIT;
+   const char *path_current = NULL;
+   const char *path_src = NULL;
+   char key[3];
+   char submodule_token[5];
+   char sep_char, eol_char;
+
+   wt_porcelain_v2_fix_up_changed(it, s);
+   wt_porcelain_v2_submodule_state(d, submodule_token);
+
+   key[0] = d->index_status ? d->index_status : '.';
+   key[1] = d->worktree_status ? d->worktree_status : '.';
+   key[2] = 0;
+
+   if (s->null_termination) {
+   /*
+* In -z mode, we DO NOT C-Quote pathnames.  Current path is 
ALWAYS first.
+* A single NUL character separates them.
+*/
+   sep_char = '\0';
+   eol_char = '\0';
+   path_current = it->string;
+   path_src = d->head_path;
+   } else {
+   /*
+* Path(s) are C-Quoted if necessary. Current path is ALWAYS 
first.
+* The source path is only present when necessary.
+* A single TAB separates them (because paths can contain spaces
+* which are not escaped and C-Quoting does escape TAB 
characters).
+*/
+   sep_char = '\t';
+   eol_char = '\n';
+   path_current = 

[PATCH v3 7/8] status: update git-status.txt for --porcelain=v2

[Jeff Hostetler]

From: Jeff Hostetler 

Update status manpage to include information about
porcelain v2 format.

Signed-off-by: Jeff Hostetler 
Signed-off-by: Jeff Hostetler 
---
 Documentation/git-status.txt | 93 ++--
 1 file changed, 90 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 6b1454b..ed3590d 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -185,10 +185,10 @@ If -b is used the short-format status is preceded by a 
line
 
 ## branchname tracking info
 
-Porcelain Format
-
+Porcelain Format Version 1
+~~
 
-The porcelain format is similar to the short format, but is guaranteed
+Version 1 porcelain format is similar to the short format, but is guaranteed
 not to change in a backwards-incompatible way between Git versions or
 based on user configuration. This makes it ideal for parsing by scripts.
 The description of the short format above also describes the porcelain
@@ -210,6 +210,93 @@ field from the first filename).  Third, filenames 
containing special
 characters are not specially formatted; no quoting or
 backslash-escaping is performed.
 
+Porcelain Format Version 2
+~~
+
+Version 2 format adds more detailed information about the state of
+the worktree and changed items.
+
+If `--branch` is given, a series of header lines are printed with
+information about the current branch.
+
+Line Notes
+
+# branch.oid  | (initial)Current commit
+# branch.head  | (detached)  Current branch
+# branch.upstream   If set
+# branch.ab + -   If set and present
+
+
+A series of lines are then displayed for the tracked entries.
+Ordinary changed entries have the following format:
+
+1
+
+Renamed or copied entries have the following format:
+
+2 \t
+
+Field   Meaning
+
+A 2 character field containing the staged and
+unstaged XY values described in the short format,
+with unchanged indicated by a "." rather than
+a space.
+   A 4 character field describing the submodule state.
+"N..." when the entry is not a submodule.
+"S" when the entry is a submodule.
+ is "C" if the commit changed; otherwise ".".
+ is "M" if it has tracked changes; otherwise ".".
+ is "U" if there are untracked changes; otherwise ".".
+The 6 character octal file mode in the HEAD.
+The octal file mode in the index.
+The octal file mode in the worktree.
+The SHA1 value in the HEAD.
+The SHA1 value in the index.
+ The rename or copied percentage score. For example "R100"
+or "C75".
+  The current pathname.
+   The original path. This is only present when the entry
+has been renamed or copied.
+
+
+Unmerged entries have the following format; the first character is
+a "u" to distinguish from ordinary changed entries.
+
+u  
+
+Field   Meaning
+
+A 2 character field describing the conflict type
+as described in the short format.
+   A 4 character field describing the submodule state
+as described above.
+The octal file mode for stage 1.
+The octal file mode for stage 2.
+The octal file mode for stage 3.
+The octal file mode in the worktree.
+The SHA1 value for stage 1.
+The SHA1 value for stage 2.
+The SHA1 value for stage 3.
+  The current pathname.
+
+
+A series of lines are then displayed for untracked and ignored entries.
+
+ 
+
+Where  is "?" for untracked entries and "!" for ignored entries.
+
+In all 3 line formats, pathnames will be "C Quoted" if they contain
+any of the following characters: TAB, LF, double quotes, or backslashes.
+These characters will be replaced with \t, \n, \", and \\, respectively,
+and the pathname will be enclosed in double quotes.
+
+When the `-z` option is given, a NUL (zero) byte follows each pathname;
+serving as both a separator and line termination. No pathname quoting
+or backslash escaping is performed. All fields are output in the same
+order.
+
 CONFIGURATION
 -
 
-- 
2.8.0.rc4.17.gac42084.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message 

[PATCH v3 8/8] status: tests for --porcelain=v2

[Jeff Hostetler]

From: Jeff Hostetler 

Unit tests for porcelain v2 status format.

Signed-off-by: Jeff Hostetler 
Signed-off-by: Jeff Hostetler 
---
 t/t7064-wtstatus-pv2.sh | 585 
 1 file changed, 585 insertions(+)
 create mode 100755 t/t7064-wtstatus-pv2.sh

diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh
new file mode 100755
index 000..ff0dd3d
--- /dev/null
+++ b/t/t7064-wtstatus-pv2.sh
@@ -0,0 +1,585 @@
+#!/bin/sh
+
+test_description='git status --porcelain=v2
+
+This test exercises porcelain V2 output for git status.'
+
+
+. ./test-lib.sh
+
+test_expect_success setup '
+   test_tick &&
+   git config --local core.autocrlf false &&
+   echo x >file_x &&
+   echo y >file_y &&
+   echo z >file_z &&
+   mkdir dir1 &&
+   echo a >dir1/file_a &&
+   echo b >dir1/file_b
+'
+
+
+##
+## Confirm output prior to initial commit.
+##
+
+test_expect_success pre_initial_commit_0 '
+   cat >expected <<-EOF &&
+   # branch.oid (initial)
+   # branch.head master
+   ? actual
+   ? dir1/
+   ? expected
+   ? file_x
+   ? file_y
+   ? file_z
+   EOF
+
+   git status --porcelain=v2 --branch --ignored --untracked-files=normal 
>actual &&
+   test_cmp expected actual
+'
+
+
+test_expect_success pre_initial_commit_1 '
+   git add file_x file_y file_z dir1 &&
+   SHA_A=`git hash-object -t blob -- dir1/file_a` &&
+   SHA_B=`git hash-object -t blob -- dir1/file_b` &&
+   SHA_X=`git hash-object -t blob -- file_x` &&
+   SHA_Y=`git hash-object -t blob -- file_y` &&
+   SHA_Z=`git hash-object -t blob -- file_z` &&
+   SHA_ZERO= &&
+
+   cat >expected <<-EOF &&
+   # branch.oid (initial)
+   # branch.head master
+   1 A. N... 00 100644 100644 $SHA_ZERO $SHA_A dir1/file_a
+   1 A. N... 00 100644 100644 $SHA_ZERO $SHA_B dir1/file_b
+   1 A. N... 00 100644 100644 $SHA_ZERO $SHA_X file_x
+   1 A. N... 00 100644 100644 $SHA_ZERO $SHA_Y file_y
+   1 A. N... 00 100644 100644 $SHA_ZERO $SHA_Z file_z
+   ? actual
+   ? expected
+   EOF
+
+   git status --porcelain=v2 --branch --ignored --untracked-files=all 
>actual &&
+   test_cmp expected actual
+'
+
+## Try -z on the above
+test_expect_success pre_initial_commit_2 '
+   cat >expected.lf <<-EOF &&
+   # branch.oid (initial)
+   # branch.head master
+   1 A. N... 00 100644 100644 $SHA_ZERO $SHA_A dir1/file_a
+   1 A. N... 00 100644 100644 $SHA_ZERO $SHA_B dir1/file_b
+   1 A. N... 00 100644 100644 $SHA_ZERO $SHA_X file_x
+   1 A. N... 00 100644 100644 $SHA_ZERO $SHA_Y file_y
+   1 A. N... 00 100644 100644 $SHA_ZERO $SHA_Z file_z
+   ? actual
+   ? expected
+   EOF
+   perl -pe y/\\012/\\000/ expected &&
+   rm expected.lf &&
+
+   git status -z --porcelain=v2 --branch --ignored --untracked-files=all 
>actual &&
+   test_cmp expected actual
+'
+
+##
+## Create first commit. Confirm commit sha in new track header.
+## Then make some changes on top of it.
+##
+
+test_expect_success initial_commit_0 '
+   git commit -m initial &&
+   H0=`git rev-parse HEAD` &&
+   cat >expected <<-EOF &&
+   # branch.oid $H0
+   # branch.head master
+   ? actual
+   ? expected
+   EOF
+
+   git status --porcelain=v2 --branch --ignored --untracked-files=all 
>actual &&
+   test_cmp expected actual
+'
+
+
+test_expect_success initial_commit_1 '
+   echo x >>file_x &&
+   SHA_X1=`git hash-object -t blob -- file_x` &&
+   rm file_z &&
+   H0=`git rev-parse HEAD` &&
+
+   cat >expected <<-EOF &&
+   # branch.oid $H0
+   # branch.head master
+   1 .M N... 100644 100644 100644 $SHA_X $SHA_X file_x
+   1 .D N... 100644 100644 00 $SHA_Z $SHA_Z file_z
+   ? actual
+   ? expected
+   EOF
+
+   git status --porcelain=v2 --branch --ignored --untracked-files=all 
>actual &&
+   test_cmp expected actual
+'
+
+
+test_expect_success initial_commit_2 '
+   git add file_x &&
+   git rm file_z &&
+   H0=`git rev-parse HEAD` &&
+
+   cat >expected <<-EOF &&
+   # branch.oid $H0
+   # branch.head master
+   1 M. N... 100644 100644 100644 $SHA_X $SHA_X1 file_x
+   1 D. N... 100644 00 00 $SHA_Z $SHA_ZERO file_z
+   ? actual
+   ? expected
+   EOF
+
+   git status --porcelain=v2 --branch --ignored --untracked-files=all 
>actual &&
+   test_cmp expected actual
+'
+
+
+test_expect_success initial_commit_3 '
+ 

[PATCH v3 6/8] status: print branch info with --porcelain=v2 --branch

[Jeff Hostetler]

From: Jeff Hostetler 

Expand porcelain v2 output to include branch and tracking
branch information.  This includes the commit SHA, the branch,
the upstream branch, and the ahead and behind counts.

Signed-off-by: Jeff Hostetler 
Signed-off-by: Jeff Hostetler 
---
 builtin/commit.c |  5 
 wt-status.c  | 90 
 wt-status.h  |  1 +
 3 files changed, 96 insertions(+)

diff --git a/builtin/commit.c b/builtin/commit.c
index 93ce28c..b1fd2d1 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -510,6 +510,8 @@ static int run_status(FILE *fp, const char *index_file, 
const char *prefix, int
s->fp = fp;
s->nowarn = nowarn;
s->is_initial = get_sha1(s->reference, sha1) ? 1 : 0;
+   if (!s->is_initial)
+   hashcpy(s->sha1_commit, sha1);
s->status_format = status_format;
s->ignore_submodule_arg = ignore_submodule_arg;
 
@@ -1378,6 +1380,9 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
fd = hold_locked_index(_lock, 0);
 
s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0;
+   if (!s.is_initial)
+   hashcpy(s.sha1_commit, sha1);
+
s.ignore_submodule_arg = ignore_submodule_arg;
s.status_format = status_format;
s.verbose = verbose;
diff --git a/wt-status.c b/wt-status.c
index 46061d4..592fbd2 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1814,6 +1814,92 @@ static void wt_porcelain_print(struct wt_status *s)
 }
 
 /*
+ * Print branch information for porcelain v2 output.  These lines
+ * are printed when the '--branch' parameter is given.
+ *
+ *# branch.oid 
+ *# branch.head 
+ *   [# branch.upstream 
+ *   [# branch.ab + -]]
+ *
+ *   ::= the current commit hash or the the literal
+ *   "(initial)" to indicate an initialized repo
+ *   with no commits.
+ *
+ * ::=  the current branch name or
+ *   "(detached)" literal when detached head or
+ *   "(unknown)" when something is wrong.
+ *
+ * ::= the upstream branch name, when set.
+ *
+ *::= integer ahead value, when upstream set
+ *   and the commit is present (not gone).
+ *
+ *   ::= integer behind value, when upstream set
+ *   and commit is present.
+ *
+ *
+ * The end-of-line is defined by the -z flag.
+ *
+ *  ::= NUL when -z,
+ *   LF when NOT -z.
+ *
+ */
+static void wt_porcelain_v2_print_tracking(struct wt_status *s)
+{
+   struct branch *branch;
+   const char *base;
+   const char *branch_name;
+   struct wt_status_state state;
+   int ab_info, nr_ahead, nr_behind;
+   char eol = s->null_termination ? '\0' : '\n';
+
+   memset(, 0, sizeof(state));
+   wt_status_get_state(, s->branch && !strcmp(s->branch, "HEAD"));
+
+   fprintf(s->fp, "# branch.oid %s%c",
+   (s->is_initial ? "(initial)" : 
sha1_to_hex(s->sha1_commit)),
+   eol);
+
+   if (!s->branch)
+   fprintf(s->fp, "# branch.head %s%c", "(unknown)", eol);
+   else {
+   if (!strcmp(s->branch, "HEAD")) {
+   fprintf(s->fp, "# branch.head %s%c", "(detached)", eol);
+
+   if (state.rebase_in_progress || 
state.rebase_interactive_in_progress)
+   branch_name = state.onto;
+   else if (state.detached_from)
+   branch_name = state.detached_from;
+   else
+   branch_name = "";
+   } else {
+   branch_name = NULL;
+   skip_prefix(s->branch, "refs/heads/", _name);
+
+   fprintf(s->fp, "# branch.head %s%c", branch_name, eol);
+   }
+
+   /* Lookup stats on the upstream tracking branch, if set. */
+   branch = branch_get(branch_name);
+   base = NULL;
+   ab_info = (stat_tracking_info(branch, _ahead, _behind, 
) == 0);
+   if (base) {
+   base = shorten_unambiguous_ref(base, 0);
+   fprintf(s->fp, "# branch.upstream %s%c", base, eol);
+   free((char *)base);
+
+   if (ab_info)
+   fprintf(s->fp, "# branch.ab +%d -%d%c", 
nr_ahead, nr_behind, eol);
+   }
+   }
+
+   free(state.branch);
+   free(state.onto);
+   free(state.detached_from);
+}
+
+/*
  * Convert various submodule status values into a
  * fixed-length string of characters in the buffer provided.
  */
@@ -2057,6 +2143,7 @@ static void wt_porcelain_v2_print_other(
 /*
  * Print porcelain V2 status.
  *
+ * []
  * []*
  * []*
  * []*
@@ -2069,6 +2156,9 @@ void 

[PATCH v3 1/8] status: rename long-format print routines

[Jeff Hostetler]

From: Jeff Hostetler 

Renamed the various wt_status_print*() routines to be
wt_longstatus_print*() to make it clear that these
routines are only concerned with the normal/long
status output.

This will hopefully reduce confusion as other status
formats are added in the future.

Signed-off-by: Jeff Hostetler 
Signed-off-by: Jeff Hostetler 
---
 builtin/commit.c |   4 +-
 wt-status.c  | 110 +++
 wt-status.h  |   2 +-
 3 files changed, 58 insertions(+), 58 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 1f6dbcd..b80273b 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -515,7 +515,7 @@ static int run_status(FILE *fp, const char *index_file, 
const char *prefix, int
break;
case STATUS_FORMAT_NONE:
case STATUS_FORMAT_LONG:
-   wt_status_print(s);
+   wt_longstatus_print(s);
break;
}
 
@@ -1403,7 +1403,7 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
case STATUS_FORMAT_LONG:
s.verbose = verbose;
s.ignore_submodule_arg = ignore_submodule_arg;
-   wt_status_print();
+   wt_longstatus_print();
break;
}
return 0;
diff --git a/wt-status.c b/wt-status.c
index de62ab2..b9a58fd 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -139,7 +139,7 @@ void wt_status_prepare(struct wt_status *s)
s->display_comment_prefix = 0;
 }
 
-static void wt_status_print_unmerged_header(struct wt_status *s)
+static void wt_longstatus_print_unmerged_header(struct wt_status *s)
 {
int i;
int del_mod_conflict = 0;
@@ -191,7 +191,7 @@ static void wt_status_print_unmerged_header(struct 
wt_status *s)
status_printf_ln(s, c, "%s", "");
 }
 
-static void wt_status_print_cached_header(struct wt_status *s)
+static void wt_longstatus_print_cached_header(struct wt_status *s)
 {
const char *c = color(WT_STATUS_HEADER, s);
 
@@ -207,9 +207,9 @@ static void wt_status_print_cached_header(struct wt_status 
*s)
status_printf_ln(s, c, "%s", "");
 }
 
-static void wt_status_print_dirty_header(struct wt_status *s,
-int has_deleted,
-int has_dirty_submodules)
+static void wt_longstatus_print_dirty_header(struct wt_status *s,
+int has_deleted,
+int has_dirty_submodules)
 {
const char *c = color(WT_STATUS_HEADER, s);
 
@@ -226,9 +226,9 @@ static void wt_status_print_dirty_header(struct wt_status 
*s,
status_printf_ln(s, c, "%s", "");
 }
 
-static void wt_status_print_other_header(struct wt_status *s,
-const char *what,
-const char *how)
+static void wt_longstatus_print_other_header(struct wt_status *s,
+const char *what,
+const char *how)
 {
const char *c = color(WT_STATUS_HEADER, s);
status_printf_ln(s, c, "%s:", what);
@@ -238,7 +238,7 @@ static void wt_status_print_other_header(struct wt_status 
*s,
status_printf_ln(s, c, "%s", "");
 }
 
-static void wt_status_print_trailer(struct wt_status *s)
+static void wt_longstatus_print_trailer(struct wt_status *s)
 {
status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", "");
 }
@@ -304,8 +304,8 @@ static int maxwidth(const char *(*label)(int), int minval, 
int maxval)
return result;
 }
 
-static void wt_status_print_unmerged_data(struct wt_status *s,
- struct string_list_item *it)
+static void wt_longstatus_print_unmerged_data(struct wt_status *s,
+ struct string_list_item *it)
 {
const char *c = color(WT_STATUS_UNMERGED, s);
struct wt_status_change_data *d = it->util;
@@ -331,9 +331,9 @@ static void wt_status_print_unmerged_data(struct wt_status 
*s,
strbuf_release();
 }
 
-static void wt_status_print_change_data(struct wt_status *s,
-   int change_type,
-   struct string_list_item *it)
+static void wt_longstatus_print_change_data(struct wt_status *s,
+   int change_type,
+   struct string_list_item *it)
 {
struct wt_status_change_data *d = it->util;
const char *c = color(change_type, s);
@@ -378,7 +378,7 @@ static void wt_status_print_change_data(struct wt_status *s,
status = d->worktree_status;
break;
default:
-   die("BUG: unhandled change_type %d in 
wt_status_print_change_data",
+   die("BUG: unhandled 

[PATCH v3 2/8] status: cleanup API to wt_status_print

[Jeff Hostetler]

From: Jeff Hostetler 

Refactor the API between builtin/commit.c and wt-status.[ch].
Hide details of the various wt_*status_print() routines inside
wt-status.c behind a single (new) wt_status_print() routine
and eliminate the switch statements from builtin/commit.c

This will allow us to more easily add new status formats
and isolate that within wt-status.c

Signed-off-by: Jeff Hostetler 
Signed-off-by: Jeff Hostetler 
---
 builtin/commit.c | 51 +--
 wt-status.c  | 25 ++---
 wt-status.h  | 16 
 3 files changed, 43 insertions(+), 49 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index b80273b..a792deb 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -142,14 +142,7 @@ static int show_ignored_in_status, have_option_m;
 static const char *only_include_assumed;
 static struct strbuf message = STRBUF_INIT;
 
-static enum status_format {
-   STATUS_FORMAT_NONE = 0,
-   STATUS_FORMAT_LONG,
-   STATUS_FORMAT_SHORT,
-   STATUS_FORMAT_PORCELAIN,
-
-   STATUS_FORMAT_UNSPECIFIED
-} status_format = STATUS_FORMAT_UNSPECIFIED;
+static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
 
 static int opt_parse_m(const struct option *opt, const char *arg, int unset)
 {
@@ -500,24 +493,11 @@ static int run_status(FILE *fp, const char *index_file, 
const char *prefix, int
s->fp = fp;
s->nowarn = nowarn;
s->is_initial = get_sha1(s->reference, sha1) ? 1 : 0;
+   s->status_format = status_format;
+   s->ignore_submodule_arg = ignore_submodule_arg;
 
wt_status_collect(s);
-
-   switch (status_format) {
-   case STATUS_FORMAT_SHORT:
-   wt_shortstatus_print(s);
-   break;
-   case STATUS_FORMAT_PORCELAIN:
-   wt_porcelain_print(s);
-   break;
-   case STATUS_FORMAT_UNSPECIFIED:
-   die("BUG: finalize_deferred_config() should have been called");
-   break;
-   case STATUS_FORMAT_NONE:
-   case STATUS_FORMAT_LONG:
-   wt_longstatus_print(s);
-   break;
-   }
+   wt_status_print(s);
 
return s->commitable;
 }
@@ -1099,7 +1079,7 @@ static const char *read_commit_message(const char *name)
  * is not in effect here.
  */
 static struct status_deferred_config {
-   enum status_format status_format;
+   enum wt_status_format status_format;
int show_branch;
 } status_deferred_config = {
STATUS_FORMAT_UNSPECIFIED,
@@ -1381,6 +1361,9 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
 
s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0;
s.ignore_submodule_arg = ignore_submodule_arg;
+   s.status_format = status_format;
+   s.verbose = verbose;
+
wt_status_collect();
 
if (0 <= fd)
@@ -1389,23 +1372,7 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
if (s.relative_paths)
s.prefix = prefix;
 
-   switch (status_format) {
-   case STATUS_FORMAT_SHORT:
-   wt_shortstatus_print();
-   break;
-   case STATUS_FORMAT_PORCELAIN:
-   wt_porcelain_print();
-   break;
-   case STATUS_FORMAT_UNSPECIFIED:
-   die("BUG: finalize_deferred_config() should have been called");
-   break;
-   case STATUS_FORMAT_NONE:
-   case STATUS_FORMAT_LONG:
-   s.verbose = verbose;
-   s.ignore_submodule_arg = ignore_submodule_arg;
-   wt_longstatus_print();
-   break;
-   }
+   wt_status_print();
return 0;
 }
 
diff --git a/wt-status.c b/wt-status.c
index b9a58fd..a9031e4 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1447,7 +1447,7 @@ static void wt_longstatus_print_state(struct wt_status *s,
show_bisect_in_progress(s, state, state_color);
 }
 
-void wt_longstatus_print(struct wt_status *s)
+static void wt_longstatus_print(struct wt_status *s)
 {
const char *branch_color = color(WT_STATUS_ONBRANCH, s);
const char *branch_status_color = color(WT_STATUS_HEADER, s);
@@ -1714,7 +1714,7 @@ static void wt_shortstatus_print_tracking(struct 
wt_status *s)
fputc(s->null_termination ? '\0' : '\n', s->fp);
 }
 
-void wt_shortstatus_print(struct wt_status *s)
+static void wt_shortstatus_print(struct wt_status *s)
 {
int i;
 
@@ -1746,7 +1746,7 @@ void wt_shortstatus_print(struct wt_status *s)
}
 }
 
-void wt_porcelain_print(struct wt_status *s)
+static void wt_porcelain_print(struct wt_status *s)
 {
s->use_color = 0;
s->relative_paths = 0;
@@ -1754,3 +1754,22 @@ void wt_porcelain_print(struct wt_status *s)
s->no_gettext = 1;
wt_shortstatus_print(s);
 }
+
+void wt_status_print(struct wt_status *s)
+{
+   switch 

[PATCH v3 0/8] status: V2 porcelain status

[Jeff Hostetler]

This patch series adds porcelain V2 format to status.
This provides detailed information about file changes
and about the current branch.

The new output is accessed via:
git status --porcelain=v2 [--branch]

Relative to the v2 patch series, in this v3 patch series
I have changed the format of the lines for ordinary changes
to make it easier to distinguish renames and copies from
normal entries.  I also split the --branch header information
onto separate lines, since we're not bound by any requirement
to have a single ## header line.  I document the argument
as --porcelain=v2, but silently also allow --porcelain=2.


Jeff Hostetler (8):
  status: rename long-format print routines
  status: cleanup API to wt_status_print
  status: support --porcelain[=]
  status: per-file data collection for --porcelain=v2
  status: print per-file porcelain v2 status data
  status: print branch info with --porcelain=v2 --branch
  status: update git-status.txt for --porcelain=v2
  status: tests for --porcelain=v2

 Documentation/git-status.txt | 100 +++-
 builtin/commit.c |  78 +++---
 t/t7060-wtstatus.sh  |  21 ++
 t/t7064-wtstatus-pv2.sh  | 585 +++
 wt-status.c  | 567 -
 wt-status.h  |  19 +-
 6 files changed, 1260 insertions(+), 110 deletions(-)
 create mode 100755 t/t7064-wtstatus-pv2.sh

-- 
2.8.0.rc4.17.gac42084.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 3/8] status: support --porcelain[=]

[Jeff Hostetler]

From: Jeff Hostetler 

Update --porcelain argument to take optional version parameter
to allow multiple porcelain formats to be supported in the future.

The token "v1" is the default value and indicates the traditional
porcelain format.  (The token "1" is an alias for that.)

Signed-off-by: Jeff Hostetler 
Signed-off-by: Jeff Hostetler 
---
 Documentation/git-status.txt |  7 +--
 builtin/commit.c | 21 ++---
 t/t7060-wtstatus.sh  | 21 +
 3 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index e1e8f57..6b1454b 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -32,11 +32,14 @@ OPTIONS
 --branch::
Show the branch and tracking info even in short-format.
 
---porcelain::
+--porcelain[=]::
Give the output in an easy-to-parse format for scripts.
This is similar to the short output, but will remain stable
across Git versions and regardless of user configuration. See
below for details.
++
+The version parameter is used to specify the format version.
+This is optional and defaults to the original version 'v1' format.
 
 --long::
Give the output in the long-format. This is the default.
@@ -96,7 +99,7 @@ configuration variable documented in linkgit:git-config[1].
 
 -z::
Terminate entries with NUL, instead of LF.  This implies
-   the `--porcelain` output format if no other format is given.
+   the `--porcelain=v1` output format if no other format is given.
 
 --column[=]::
 --no-column::
diff --git a/builtin/commit.c b/builtin/commit.c
index a792deb..c3ae2c3 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -144,6 +144,21 @@ static struct strbuf message = STRBUF_INIT;
 
 static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
 
+static int opt_parse_porcelain(const struct option *opt, const char *arg, int 
unset)
+{
+   enum wt_status_format *value = (enum wt_status_format *)opt->value;
+   if (unset)
+   *value = STATUS_FORMAT_NONE;
+   else if (!arg)
+   *value = STATUS_FORMAT_PORCELAIN;
+   else if (!strcmp(arg, "v1") || !strcmp(arg,"1"))
+   *value = STATUS_FORMAT_PORCELAIN;
+   else
+   die("unsupported porcelain version '%s'", arg);
+
+   return 0;
+}
+
 static int opt_parse_m(const struct option *opt, const char *arg, int unset)
 {
struct strbuf *buf = opt->value;
@@ -1316,9 +1331,9 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
N_("show status concisely"), STATUS_FORMAT_SHORT),
OPT_BOOL('b', "branch", _branch,
 N_("show branch information")),
-   OPT_SET_INT(0, "porcelain", _format,
-   N_("machine-readable output"),
-   STATUS_FORMAT_PORCELAIN),
+   { OPTION_CALLBACK, 0, "porcelain", _format,
+ N_("version"), N_("machine-readable output"),
+ PARSE_OPT_OPTARG, opt_parse_porcelain },
OPT_SET_INT(0, "long", _format,
N_("show status in long format (default)"),
STATUS_FORMAT_LONG),
diff --git a/t/t7060-wtstatus.sh b/t/t7060-wtstatus.sh
index 44bf1d8..00e0ceb 100755
--- a/t/t7060-wtstatus.sh
+++ b/t/t7060-wtstatus.sh
@@ -228,4 +228,25 @@ test_expect_success 'status --branch with detached HEAD' '
test_i18ncmp expected actual
 '
 
+## Duplicate the above test and verify --porcelain=v1 arg parsing.
+test_expect_success 'status --porcelain=v1 --branch with detached HEAD' '
+   git reset --hard &&
+   git checkout master^0 &&
+   git status --branch --porcelain=v1 >actual &&
+   cat >expected <<-EOF &&
+   ## HEAD (no branch)
+   ?? .gitconfig
+   ?? actual
+   ?? expect
+   ?? expected
+   ?? mdconflict/
+   EOF
+   test_i18ncmp expected actual
+'
+
+## Verify parser error on invalid --porcelain argument.
+test_expect_success 'status --porcelain=bogus' '
+   test_must_fail git status --porcelain=bogus
+'
+
 test_done
-- 
2.8.0.rc4.17.gac42084.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] merge: Run commit-msg hook

[Junio C Hamano]

Johannes Schindelin  writes:

> FWIW I dug out the original submission:
> http://thread.gmane.org/gmane.comp.version-control.git/151297/focus=151435
>
> It seems that there was no discussion about the commit-msg. Which makes me
> wonder why nobody thought of this.

I actually think it was a mistaken argument.

We could have devised a mechanism to prevent "git commit" that
concludes a conflicted "git merge" from calling the hook to resolve
the inconsistency, and the solution would have been equally valid.

I'd rather not make things worse by repeating that same mistake.  If
we were to change anything, we should be adding prepare-merge-msg as
you suggested earlier and weaning people off of the current
behaviour that was added by mistake.

Thanks.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] merge: Run commit-msg hook

[Junio C Hamano]

Orgad Shaneh  writes:

>> Hmm. This is not very convincing to me, as
>>
>> - if you call commit-msg in `git merge` now, why not `prepare-commit-msg`?
>
> prepare-commit-msg is already called, a few lines above this addition.

I do not see such call in contrib/example/git-merge.sh; could it be
a recent bug that it calls it?

>> - a merge is a different beast from a simple commit. That is why we have
>>   two different commands for them. A hook to edit the merge message may
>>   need to know the *second* parent commit, too, for example to generate
>>   a diffstat, or to add information about changes in an "evil commit".
>
> That is correct for a post-merge hook. Why should *message validation* differ
> between simple and merge commit?

Have you seen a typical merge commit message?  They certainly look
different to me and different rules would apply.

>> - if Gerrit is the intended user, would it not make more sense to
>>   introduce a new hook, e.g. `merge-msg` (and `prepare-merge-msg`), as you
>>   have to teach Gerrit a new trick anyway?
>
> Why is that new? Every commit in gerrit has a Change-Id footer, which is
> generated by commit-msg hook.

Hmm, I didn't know Gerrit gave Change-ID to merges.

In any case, I would think this is completely new.  Without this
change, commit-msg was not called for merges, so Gerrit wouldn't
have added Change-ID to merges with that mechanism.

A new merge-msg hook would make more sense, if we were making any
change.  I do not want to get yelled at by those who wrote their
commit-msg hooks long time ago and have happily been using them that
updated Git started calling them for their merges where their
validation logic for their single-parent commit do not apply at all.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] subtree: adjust style to match CodingGuidelines

[Junio C Hamano]

Johannes Sixt  writes:

> These caught my eye browsing through my inbox. I'm not a subtree user.

All good comments.

Let's queue 1/3 and 2/3 and fast-track them down to 'master'.  Style
fixes can come independently later.

Thanks.

> Am 26.07.2016 um 06:14 schrieb David Aguilar:
>> @@ -50,87 +51,145 @@ prefix=
>>
>>  debug()
>>  {
>> -if [ -n "$debug" ]; then
>> -printf "%s\n" "$*" >&2
>> +if test -n "$debug"
>> +then
>> +printf "%s\n" "$@" >&2
>
> Are you sure you want this? It prints each argument of the 'debug'
> invocation on its own line.
>
>>  fi
>>  }
>>
>>  say()
>>  {
>> -if [ -z "$quiet" ]; then
>> -printf "%s\n" "$*" >&2
>> +if test -z "$quiet"
>> +then
>> +printf "%s\n" "$@" >&2
>
> Same here.
>
>>  fi
>>  }
>>
>>  progress()
>>  {
>> -if [ -z "$quiet" ]; then
>> -printf "%s\r" "$*" >&2
>> +if test -z "$quiet"
>> +then
>> +printf "%s\r" "$@" >&2
>
> But here I'm pretty sure that this is not wanted; the original is
> clearly correct.
>
>>  fi
>>  }
> ...
>> @@ -139,22 +198,27 @@ debug "command: {$command}"
>>  debug "quiet: {$quiet}"
>>  debug "revs: {$revs}"
>>  debug "dir: {$dir}"
>> -debug "opts: {$*}"
>> +debug "opts: {$@}"
>
> When the arguments of a script or function are to be printed for the
> user's entertainment/education, then it is safer (and, therefore,
> idiomatic) to use "$*".
>
>>  debug
> ...
>>  cache_get()
>>  {
>> -for oldrev in $*; do
>> -if [ -r "$cachedir/$oldrev" ]; then
>> +for oldrev in "$@"
>> +do
>
> It is idiomatic to write this as
>
>   for oldrev
>   do
>
> (But your move from bare $* to quoted "$@" fits better under the "fix
> quoting" topic of this patch.)
>
>> +if test -r "$cachedir/$oldrev"
>> +then
>>  read newrev <"$cachedir/$oldrev"
>>  echo $newrev
>>  fi
> ...
>> @@ -631,17 +749,19 @@ cmd_split()
>>  debug "  parents: $parents"
>>  newparents=$(cache_get $parents)
>>  debug "  newparents: $newparents"
>> -
>> +
>>  tree=$(subtree_for_commit $rev "$dir")
>>  debug "  tree is: $tree"
>>
>>  check_parents $parents
>> -
>> +
>>  # ugly.  is there no better way to tell if this is a subtree
>>  # vs. a mainline commit?  Does it matter?
>> -if [ -z $tree ]; then
>> +if test -z $tree
>
> This works by accident. When $tree is empty, this reduces to 'test
> -z', which happens to evaluate to true, just what we want. But it be
> appropriate to put $tree in double-quotes nevertheless.
>
>> +then
>>  set_notree $rev
>> -if [ -n "$newparents" ]; then
>> +if test -n "$newparents"
>> +then
>>  cache_set $rev $rev
>>  fi
>>  continue
>
> -- Hannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH]submodule deinit: remove outdated comment

[Junio C Hamano]

Stefan Beller  writes:

> Signed-off-by: Stefan Beller 
> ---
>
>   This is logically part of origin/sb/submodule-deinit-all, but this change
>   failed to be there on time.

Thanks; let's queue it there and fast-track it down to 'master'.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] pack-objects: break out of want_object loop early

[Jeff King]

On Tue, Jul 26, 2016 at 01:38:47PM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> >> I do not mind too much about having to check two bools twice.  But
> >> given that the reason why I was confused was because I didn't see
> >> why we need to pass the two "return 0" conditions at least once
> >> before we decide that we do not need the "return 0" thing at all,
> >> and started constructing a case where this might break by writing
> >> "Suppose you have two packs, one remote and one local in packed_git
> >> list in this order, and ..." before I realized that the new "early
> >> break" can be hoisted up like the above, I definitely feel that "we
> >> found one, and we aren't conditionally pretending that this thing
> >> does not need to be packed at all, so return early and say we want
> >> to pack it" is easier to understand before the two existing "if"
> >> statements.
> >
> > Ah, right. Now you had me second-guessing for a moment that there might
> > be a bad case in hoisting it up where we would want to return 0 but
> > would break out early to the "return 1".
> >
> > But it cannot be the case, because the break is mutually exclusive with
> > the two conditions.
> 
> Here is what I amended looks like (with s/local/non-local/ in the
> log message).

Thanks, I was actually just preparing a very similar patch (to move the
condition and to add a comment, since clearly it is tricky).

I got side-tracked by adding a t/perf test to show off the improvement.
It's rather tricky to get right and takes a long time to run. I _think_
I have it now, but am waiting for results. :)

> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index a2f8cfd..a46bf5b 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -977,6 +977,21 @@ static int want_object_in_pack(const unsigned char *sha1,
>   return 1;
>   if (incremental)
>   return 0;
> +
> + /*
> +  * When asked to do --local (do not include an
> +  * object that appears in a pack we borrow
> +  * from elsewhere) or --honor-pack-keep (do not
> +  * include an object that appears in a pack marked
> +  * with .keep), we need to make sure no copy of this
> +  * object come from in _any_ pack that causes us to
> +  * omit it, and need to complete this loop.  When
> +  * neither option is in effect, we know the object
> +  * we just found is going to be packed, so break
> +  * out of the loop to return 1 now.
> +  */
> + if (!ignore_packed_keep && !local)
> + break;

This looks great. Given the explanation in the comment, it might be more
clear to switch the break to "return 1", but I could go either way.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 0/3] push: allow pushing new branches with --force-with-lease

[John Keeping]

Changes in v3:
- Use hashclr() and oidclr() where appropriate instead of memset()
- Pull a test forward from patch 3 to patch 2 

John Keeping (3):
  Documentation/git-push: fix placeholder formatting
  push: add shorthand for --force-with-lease branch creation
  push: allow pushing new branches with --force-with-lease

 Documentation/git-push.txt |  5 +++--
 remote.c   |  9 +
 remote.h   |  1 -
 t/t5533-push-cas.sh| 38 ++
 4 files changed, 46 insertions(+), 7 deletions(-)

-- 
2.9.2.639.g855ae9f
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/3] Documentation/git-push: fix placeholder formatting

[John Keeping]

Format the placeholder as monospace to match other occurrences in this
file and obey CodingGuidelines.

Signed-off-by: John Keeping 
---
No changes in v3.

 Documentation/git-push.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 93c3527..bf7c9a2 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -198,7 +198,7 @@ branch we have for it.
 +
 `--force-with-lease=:` will protect the named ref (alone),
 if it is going to be updated, by requiring its current value to be
-the same as the specified value  (which is allowed to be
+the same as the specified value `` (which is allowed to be
 different from the remote-tracking branch we have for the refname,
 or we do not even have to have such a remote-tracking branch when
 this form is used).
-- 
2.9.2.639.g855ae9f

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 3/3] push: allow pushing new branches with --force-with-lease

[John Keeping]

If there is no upstream information for a branch, it is likely that it
is newly created and can safely be pushed under the normal fast-forward
rules.  Relax the --force-with-lease check so that we do not reject
these branches immediately but rather attempt to push them as new
branches, using the null SHA-1 as the expected value.

In fact, it is already possible to push new branches using the explicit
--force-with-lease=: syntax, so all we do here is make
this behaviour the default if no explicit "expect" value is specified.

Signed-off-by: John Keeping 
---
Changes in v3:
- use oidclr()
- final test is now added in the previous patch and now uses the
  explicit --force-with-lease syntax

 remote.c|  7 +++
 remote.h|  1 -
 t/t5533-push-cas.sh | 12 
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/remote.c b/remote.c
index 42c4a34..d29850a 100644
--- a/remote.c
+++ b/remote.c
@@ -1544,8 +1544,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int 
send_mirror,
 * branch.
 */
if (ref->expect_old_sha1) {
-   if (ref->expect_old_no_trackback ||
-   oidcmp(>old_oid, >old_oid_expect))
+   if (oidcmp(>old_oid, >old_oid_expect))
reject_reason = REF_STATUS_REJECT_STALE;
else
/* If the ref isn't stale then force the 
update. */
@@ -2345,7 +2344,7 @@ static void apply_cas(struct push_cas_option *cas,
if (!entry->use_tracking)
hashcpy(ref->old_oid_expect.hash, cas->entry[i].expect);
else if (remote_tracking(remote, ref->name, 
>old_oid_expect))
-   ref->expect_old_no_trackback = 1;
+   oidclr(>old_oid_expect);
return;
}
 
@@ -2355,7 +2354,7 @@ static void apply_cas(struct push_cas_option *cas,
 
ref->expect_old_sha1 = 1;
if (remote_tracking(remote, ref->name, >old_oid_expect))
-   ref->expect_old_no_trackback = 1;
+   oidclr(>old_oid_expect);
 }
 
 void apply_push_cas(struct push_cas_option *cas,
diff --git a/remote.h b/remote.h
index c21fd37..9248811 100644
--- a/remote.h
+++ b/remote.h
@@ -89,7 +89,6 @@ struct ref {
force:1,
forced_update:1,
expect_old_sha1:1,
-   expect_old_no_trackback:1,
deletion:1,
matched:1;
 
diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
index ed631c3..09899af 100755
--- a/t/t5533-push-cas.sh
+++ b/t/t5533-push-cas.sh
@@ -191,6 +191,18 @@ test_expect_success 'cover everything with default 
force-with-lease (allowed)' '
test_cmp expect actual
 '
 
+test_expect_success 'new branch covered by force-with-lease' '
+   setup_srcdst_basic &&
+   (
+   cd dst &&
+   git branch branch master &&
+   git push --force-with-lease=branch origin branch
+   ) &&
+   git ls-remote dst refs/heads/branch >expect &&
+   git ls-remote src refs/heads/branch >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'new branch covered by force-with-lease (explicit)' '
setup_srcdst_basic &&
(
-- 
2.9.2.639.g855ae9f

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/3] push: add shorthand for --force-with-lease branch creation

[John Keeping]

Allow the empty string to stand in for the null SHA-1 when pushing a new
branch, like we do when deleting branches.

This means that the following command ensures that `new-branch` is
created on the remote (that is, is must not already exist):

git push --force-with-lease=new-branch: origin new-branch

Signed-off-by: John Keeping 
---
Changes in v3:
- use hashclr()
- pull 'new branch already exists' test forward from patch 3 and use
  explicit --force-with-lease syntax

 Documentation/git-push.txt |  3 ++-
 remote.c   |  2 ++
 t/t5533-push-cas.sh| 26 ++
 3 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index bf7c9a2..927a034 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -201,7 +201,8 @@ if it is going to be updated, by requiring its current 
value to be
 the same as the specified value `` (which is allowed to be
 different from the remote-tracking branch we have for the refname,
 or we do not even have to have such a remote-tracking branch when
-this form is used).
+this form is used).  If `` is the empty string, then the named ref
+must not already exist.
 +
 Note that all forms other than `--force-with-lease=:`
 that specifies the expected current value of the ref explicitly are
diff --git a/remote.c b/remote.c
index a326e4e..42c4a34 100644
--- a/remote.c
+++ b/remote.c
@@ -2294,6 +2294,8 @@ int parse_push_cas_option(struct push_cas_option *cas, 
const char *arg, int unse
entry = add_cas_entry(cas, arg, colon - arg);
if (!*colon)
entry->use_tracking = 1;
+   else if (!colon[1])
+   hashclr(entry->expect);
else if (get_sha1(colon + 1, entry->expect))
return error("cannot parse expected object name '%s'", colon + 
1);
return 0;
diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
index c732012..ed631c3 100755
--- a/t/t5533-push-cas.sh
+++ b/t/t5533-push-cas.sh
@@ -191,4 +191,30 @@ test_expect_success 'cover everything with default 
force-with-lease (allowed)' '
test_cmp expect actual
 '
 
+test_expect_success 'new branch covered by force-with-lease (explicit)' '
+   setup_srcdst_basic &&
+   (
+   cd dst &&
+   git branch branch master &&
+   git push --force-with-lease=branch: origin branch
+   ) &&
+   git ls-remote dst refs/heads/branch >expect &&
+   git ls-remote src refs/heads/branch >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'new branch already exists' '
+   setup_srcdst_basic &&
+   (
+   cd src &&
+   git checkout -b branch master &&
+   test_commit c
+   ) &&
+   (
+   cd dst &&
+   git branch branch master &&
+   test_must_fail git push --force-with-lease=branch: origin branch
+   )
+'
+
 test_done
-- 
2.9.2.639.g855ae9f

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] commit: Fix description of no-verify

[Junio C Hamano]

org...@gmail.com writes:

> Subject: Re: [PATCH v2] commit: Fix description of no-verify

Following the prevailing style from "git shortlog --no-merges -100"
would make it "commit: fix description of no-verify", but "fix" is
too generic a word and does not convey as much information as it
wastes bits ;-)

Subject: commit: --no-verify option skips commit-msg hook, too

perhaps?

> From: Orgad Shaneh 
>
> include also commit-msg hook.

Then this half-sentence can go completely.

Thanks; I'll do the above myself while queuing, so there is no need
to resend.

> diff --git a/builtin/commit.c b/builtin/commit.c
> index 163dbca..2725712 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1616,7 +1616,7 @@ int cmd_commit(int argc, const char **argv, const char 
> *prefix)
>   OPT_BOOL(0, "interactive", , N_("interactively add 
> files")),
>   OPT_BOOL('p', "patch", _interactive, N_("interactively 
> add changes")),
>   OPT_BOOL('o', "only", , N_("commit only specified files")),
> - OPT_BOOL('n', "no-verify", _verify, N_("bypass pre-commit 
> hook")),
> + OPT_BOOL('n', "no-verify", _verify, N_("bypass pre-commit 
> and commit-msg hooks")),
>   OPT_BOOL(0, "dry-run", _run, N_("show what would be 
> committed")),
>   OPT_SET_INT(0, "short", _format, N_("show status 
> concisely"),
>   STATUS_FORMAT_SHORT),
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git cherry-pick conflict error message is deceptive when cherry-picking multiple commits

[Stephen Morton]

On Tue, Jul 26, 2016 at 4:30 PM, Jeff King  wrote:
> On Tue, Jul 26, 2016 at 01:18:55PM -0700, Stefan Beller wrote:
>
>> > Would it be possible to expand the hint message to tell users to run
>> > 'git cherry-pick --continue'
>>
>> Instead of expanding I'd go for replacing?
>>
>> I'd say the user is tempted for 2 choices,
>> a) aborting (for various reasons)
>> b) fix and continue.
>
> Yeah, I'd agree with this.
>
> I think that advice comes from a time when you could only cherry-pick a
> single commit. These days you can do several in a single run, and that's
> why "git cherry-pick --continue" was invented.
>
> So I think we would need to make sure that the "cherry-pick --continue"
> advice applies in both cases (and that we do not need to give different
> advice depending on whether we are in a single or multiple cherry-pick).
>
> I did some basic tests and it _seems_ to work to use --continue in
> either case. Probably due to 093a309 (revert: allow cherry-pick
> --continue to commit before resuming, 2011-12-10), but I didn't dig.
>
> -Peff

The 'git status' text for a rebase/am/cherry-pick is

fix conflicts and then run "git  --continue"
use "git  --skip" to skip this patch"
use "git  --abort" to cancel the  operation

(The --cancel text varies a bit actually, but that's the gist of it.)

The rebase/cherry-pick conflict case should really indicate how to
mark the conflict as resolved as that's the specific situation the
user is in. I don't know if there are guidelines to hint line length,
or how many actions should be on one line but if the above text was
changed to have this as the "fix" text, possibly over two lines, I
think that would do it.

fix conflicts with 'git add ' or 'git rm '" and then
run "git  --continue"

Stephen
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] push: add shorthand for --force-with-lease branch creation

[John Keeping]

On Tue, Jul 26, 2016 at 12:59:04PM -0700, Junio C Hamano wrote:
> John Keeping  writes:
> 
> >> > @@ -2294,6 +2294,8 @@ int parse_push_cas_option(struct push_cas_option 
> >> > *cas, const char *arg, int unse
> >> >  entry = add_cas_entry(cas, arg, colon - arg);
> >> >  if (!*colon)
> >> >  entry->use_tracking = 1;
> >> > +else if (!colon[1])
> >> > +memset(entry->expect, 0, sizeof(entry->expect));
> >> 
> >> hashclr()?
> >
> > Yes (and in the following patch as well).  I hadn't realised that
> > function exists.
> 
> Thanks; I've locally tweaked these two patches; the interdiff looks
> like this.

Thanks.  I'm about to send v3 anyway to pull a test forward to address
Jakub's comment.  I also used oidclr() for the last two changes below.

>  remote.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/remote.c b/remote.c
> index e8b7bac..7eaf3c8 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -2304,7 +2304,7 @@ int parse_push_cas_option(struct push_cas_option *cas, 
> const char *arg, int unse
>   if (!*colon)
>   entry->use_tracking = 1;
>   else if (!colon[1])
> - memset(entry->expect, 0, sizeof(entry->expect));
> + hashclr(entry->expect);
>   else if (get_sha1(colon + 1, entry->expect))
>   return error("cannot parse expected object name '%s'", colon + 
> 1);
>   return 0;
> @@ -2354,7 +2354,7 @@ static void apply_cas(struct push_cas_option *cas,
>   if (!entry->use_tracking)
>   hashcpy(ref->old_oid_expect.hash, cas->entry[i].expect);
>   else if (remote_tracking(remote, ref->name, 
> >old_oid_expect))
> - memset(>old_oid_expect, 0, 
> sizeof(ref->old_oid_expect));
> + hashclr(ref->old_oid_expect.hash);
>   return;
>   }
>  
> @@ -2364,7 +2364,7 @@ static void apply_cas(struct push_cas_option *cas,
>  
>   ref->expect_old_sha1 = 1;
>   if (remote_tracking(remote, ref->name, >old_oid_expect))
> - memset(>old_oid_expect, 0, sizeof(ref->old_oid_expect));
> + hashclr(ref->old_oid_expect.hash);
>  }
>  
>  void apply_push_cas(struct push_cas_option *cas,
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 2/3] convert: modernize tests

[Junio C Hamano]

On Tue, Jul 26, 2016 at 8:18 AM, Remi Galan Alfonso
 wrote:
> Considering how close it is to your patch, you might also want to
> remove spaces after '<'.
>
> There is only one occurrence in this file and it's in a line you are
> already modifying.

Good eyes.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] pack-objects: break out of want_object loop early

[Junio C Hamano]

Jeff King  writes:

>> I do not mind too much about having to check two bools twice.  But
>> given that the reason why I was confused was because I didn't see
>> why we need to pass the two "return 0" conditions at least once
>> before we decide that we do not need the "return 0" thing at all,
>> and started constructing a case where this might break by writing
>> "Suppose you have two packs, one remote and one local in packed_git
>> list in this order, and ..." before I realized that the new "early
>> break" can be hoisted up like the above, I definitely feel that "we
>> found one, and we aren't conditionally pretending that this thing
>> does not need to be packed at all, so return early and say we want
>> to pack it" is easier to understand before the two existing "if"
>> statements.
>
> Ah, right. Now you had me second-guessing for a moment that there might
> be a bad case in hoisting it up where we would want to return 0 but
> would break out early to the "return 1".
>
> But it cannot be the case, because the break is mutually exclusive with
> the two conditions.

Here is what I amended looks like (with s/local/non-local/ in the
log message).

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index a2f8cfd..a46bf5b 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -977,6 +977,21 @@ static int want_object_in_pack(const unsigned char *sha1,
return 1;
if (incremental)
return 0;
+
+   /*
+* When asked to do --local (do not include an
+* object that appears in a pack we borrow
+* from elsewhere) or --honor-pack-keep (do not
+* include an object that appears in a pack marked
+* with .keep), we need to make sure no copy of this
+* object come from in _any_ pack that causes us to
+* omit it, and need to complete this loop.  When
+* neither option is in effect, we know the object
+* we just found is going to be packed, so break
+* out of the loop to return 1 now.
+*/
+   if (!ignore_packed_keep && !local)
+   break;
if (local && !p->pack_local)
return 0;
if (ignore_packed_keep && p->pack_local && p->pack_keep)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git cherry-pick conflict error message is deceptive when cherry-picking multiple commits

[Jeff King]

On Tue, Jul 26, 2016 at 01:18:55PM -0700, Stefan Beller wrote:

> > Would it be possible to expand the hint message to tell users to run
> > 'git cherry-pick --continue'
> 
> Instead of expanding I'd go for replacing?
> 
> I'd say the user is tempted for 2 choices,
> a) aborting (for various reasons)
> b) fix and continue.

Yeah, I'd agree with this.

I think that advice comes from a time when you could only cherry-pick a
single commit. These days you can do several in a single run, and that's
why "git cherry-pick --continue" was invented.

So I think we would need to make sure that the "cherry-pick --continue"
advice applies in both cases (and that we do not need to give different
advice depending on whether we are in a single or multiple cherry-pick).

I did some basic tests and it _seems_ to work to use --continue in
either case. Probably due to 093a309 (revert: allow cherry-pick
--continue to commit before resuming, 2011-12-10), but I didn't dig.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git cherry-pick conflict error message is deceptive when cherry-picking multiple commits

[Stefan Beller]

> Would it be possible to expand the hint message to tell users to run
> 'git cherry-pick --continue'

Instead of expanding I'd go for replacing?

I'd say the user is tempted for 2 choices,
a) aborting (for various reasons)
b) fix and continue.

So we'd want to point out the way for those two ways and not
give an additional arbitrary way that is not quite (b) but goes that
direction?

I realize this is what the patch does, but I wanted to point out the difference
on expanding vs replacing. I think a 4 line hint is "enough", so I'd object
adding more lines, but changing existing lines is great!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] t5510: skip tests under GETTEXT_POISON build

[Ævar Arnfjörð Bjarmason]

On Tue, Jul 26, 2016 at 6:53 PM, Junio C Hamano  wrote:
> [...] Back when 5e9637c6 (i18n: add infrastructure for
> translating Git with gettext, 2011-11-18) introduced the former,
> test_have_prereq did not support a negated prerequisite, so the
> commit added GETTEXT_POISON prerequisite; if we had the modern
> test_have_prereq, we would have written
>
> test_expect_success GETTEXT_POISON '...'
>
> that appear in t0205 as
>
> test_expect_success !C_LOCALE_OUTPUT '...'
>
> I would think.

Maybe the names of the test prerequisites should be merged. I can't
think of a rea

As for the GETTEXT_POISON facility in general, I haven't worked much
if at all on the i18n toolchain since I initially wrote the gettext
support so I think at this point it's for others to say whether stuff
like this is useful.

But for what it's worth the v1.7.4.1-65-gbb946bb commit explains
better what it's for:

This is a debugging aid for people who are working on the i18n part of
the system, to make sure that they are not marking plumbing messages
that should never be translated with _().

I.e. so the person gettext-izing something can actively spot issues
with marking strings for translations right away.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 0/3] Git filter protocol

[Jeff King]

On Sun, Jul 24, 2016 at 01:24:29PM +0200, Lars Schneider wrote:

> What if we would keep the config option "protocol" and make it an "int"? 
> Undefined or version "1" would describe the existing clean/smudge 
> protocol via command line and pipe. Version "2" would be the new protocol?

FWIW, that is what I expected when I saw the word "protocol".

It's possible that we might never need a "v3" protocol specified here,
because your v2 protocol should be able to auto-upgrade. That is, if we
start a filter and it says "hi, I am speaking protocol 3", then Git
knows to speak the requested version from there on (or will barf if it
doesn't understand the version).

So you'd only need to say "filter.foo.protocol=v3" if there was some
protocol change that broke the initial conversation.

That does mean it is the filter which sets the maximum protocol level,
not git. So a filter which can speak v3 or v2 (to work with older
versions of git) does not know which to use. That could be solved by
specifying

  [filter "foo"]
  smudge = my-filter --version=3

or something.

I'm not sure it's worth thinking too hard about what-ifs here. We should
do the simplest thing that will work and avoid painting ourselves into a
corner for future upgrades.

> > * The way the serialized access to these long-running processes
> >   work in 3/3 would make it harder or impossible to later
> >   parallelize conversion?  I am imagining a far future where we
> >   would run "git checkout ." using (say) two threads, one
> >   responsible for active_cache[0..active_nr/2] and the other
> >   responsible for the remainder.
> I hope this future is not too far away :-) 
> However, I don't think that would be a problem as we could start the
> long-running process once for each checkout thread, no?

That's reasonable if we have a worker-thread model (which seems likely,
as that's what we use elsewhere in git), and if the main cost you want
to amortize is just process startup (so you pay the cost once per
worker, which is a constant factor and not too bad).

It's not a good model if the long-running process wants to amortize
other shared costs. For example, persistent https connections. Or even
user-interactive authentication steps, where you really would prefer to
do them once. The filter can implement its own ad-hoc sharing of
resources, but doing that portably is complicated.

Of course having an async protocol between git and the filter is also
complicated. Perhaps that's something that could wait for a v3 if
somebody really wants it.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 3/3] convert: add filter..useProtocol option

[Jeff King]

On Sat, Jul 23, 2016 at 07:27:21AM +, Eric Wong wrote:

> Jakub Narębski  wrote:
> > W dniu 2016-07-22 o 17:49, larsxschnei...@gmail.com pisze:
> > > +use strict;
> > > +use warnings;
> > > +use autodie;
> > 
> > autodie?
> 
> "set -e" for Perl (man autodie)
> 
> It's been a part of Perl for ages, but I've never used it
> myself, either; I suppose it's fine for tests...

autodie has been around for a long time, but it only became part of the
perl core in v5.10.1 (according to Module::CoreList). I think the code
in perl/ requires only 5.8, but whenever we unconditionally use perl
without respect to NO_PERL (like in the test scripts), we usually shoot
for even antique versions of perl like 5.005.

So by those rules, we should avoid "autodie" here, though I wouldn't be
surprised if it takes a while for people to complain in practice (most
modern systems will have a recent enough perl, but it seems we go
through cycles where every few years somebody posts a bunch of patches
for ancient versions of IRIX or some other platform, cleaning up all of
these sorts of portability problems).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] push: add shorthand for --force-with-lease branch creation

[Junio C Hamano]

John Keeping  writes:

>> > @@ -2294,6 +2294,8 @@ int parse_push_cas_option(struct push_cas_option 
>> > *cas, const char *arg, int unse
>> >entry = add_cas_entry(cas, arg, colon - arg);
>> >if (!*colon)
>> >entry->use_tracking = 1;
>> > +  else if (!colon[1])
>> > +  memset(entry->expect, 0, sizeof(entry->expect));
>> 
>> hashclr()?
>
> Yes (and in the following patch as well).  I hadn't realised that
> function exists.

Thanks; I've locally tweaked these two patches; the interdiff looks
like this.

 remote.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/remote.c b/remote.c
index e8b7bac..7eaf3c8 100644
--- a/remote.c
+++ b/remote.c
@@ -2304,7 +2304,7 @@ int parse_push_cas_option(struct push_cas_option *cas, 
const char *arg, int unse
if (!*colon)
entry->use_tracking = 1;
else if (!colon[1])
-   memset(entry->expect, 0, sizeof(entry->expect));
+   hashclr(entry->expect);
else if (get_sha1(colon + 1, entry->expect))
return error("cannot parse expected object name '%s'", colon + 
1);
return 0;
@@ -2354,7 +2354,7 @@ static void apply_cas(struct push_cas_option *cas,
if (!entry->use_tracking)
hashcpy(ref->old_oid_expect.hash, cas->entry[i].expect);
else if (remote_tracking(remote, ref->name, 
>old_oid_expect))
-   memset(>old_oid_expect, 0, 
sizeof(ref->old_oid_expect));
+   hashclr(ref->old_oid_expect.hash);
return;
}
 
@@ -2364,7 +2364,7 @@ static void apply_cas(struct push_cas_option *cas,
 
ref->expect_old_sha1 = 1;
if (remote_tracking(remote, ref->name, >old_oid_expect))
-   memset(>old_oid_expect, 0, sizeof(ref->old_oid_expect));
+   hashclr(ref->old_oid_expect.hash);
 }
 
 void apply_push_cas(struct push_cas_option *cas,
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 40/41] builtin/am: use apply api in run_apply()

[Junio C Hamano]

Christian Couder  writes:

>  static int run_apply(const struct am_state *state, const char *index_file)
>  {
> - struct child_process cp = CHILD_PROCESS_INIT;
> + struct argv_array apply_paths = ARGV_ARRAY_INIT;
> + struct argv_array apply_opts = ARGV_ARRAY_INIT;
> + struct apply_state apply_state;
> + int res, opts_left;
> + char *save_index_file;
> + static struct lock_file lock_file;
> +
> + struct option am_apply_options[] = {
> + { OPTION_CALLBACK, 0, "whitespace", _state, N_("action"),
> + N_("detect new or modified lines that have whitespace 
> errors"),
> + 0, apply_option_parse_whitespace },
> + { OPTION_CALLBACK, 0, "ignore-space-change", _state, NULL,
> + N_("ignore changes in whitespace when finding context"),
> + PARSE_OPT_NOARG, apply_option_parse_space_change },
> + { OPTION_CALLBACK, 0, "ignore-whitespace", _state, NULL,
> + N_("ignore changes in whitespace when finding context"),
> + PARSE_OPT_NOARG, apply_option_parse_space_change },
> + { OPTION_CALLBACK, 0, "directory", _state, N_("root"),
> + N_("prepend  to all filenames"),
> + 0, apply_option_parse_directory },
> + { OPTION_CALLBACK, 0, "exclude", _state, N_("path"),
> + N_("don't apply changes matching the given path"),
> + 0, apply_option_parse_exclude },
> + { OPTION_CALLBACK, 0, "include", _state, N_("path"),
> + N_("apply changes matching the given path"),
> + 0, apply_option_parse_include },
> + OPT_INTEGER('C', NULL, _state.p_context,
> + N_("ensure at least  lines of context 
> match")),
> + { OPTION_CALLBACK, 'p', NULL, _state, N_("num"),
> + N_("remove  leading slashes from traditional diff 
> paths"),
> + 0, apply_option_parse_p },
> + OPT_BOOL(0, "reject", _state.apply_with_reject,
> + N_("leave the rejected hunks in corresponding *.rej 
> files")),
> + OPT_END()
> + };

Is this an indication that this step came too prematurely?

Can we avoid having to maintain semi-duplicated options array if
cmd_apply() is properly refactored before this step?  The resulting
code is unmaintainable as long as we have this duplication.


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 7/8] status: update git-status.txt for --porcelain=v2

[Jeff Hostetler]

On 07/25/2016 06:43 PM, Jakub Narębski wrote:

W dniu 2016-07-25 o 21:25, Jeff Hostetler pisze:


+Porcelain Format Version 2
+~~
+
+Version 2 format adds more detailed information about the state of
+the worktree and the changed items.


I think it should be "and changed items", but I am not a native speaker.


fixed.


+If `--branch` is given, a header line showing branch tracking information
+is printed.  This line begins with "### branch: ".  Fields are separated
+by a single space.
+
+FieldMeaning
+
+ | (initial)Current commit
+ | (detached)Current branch
+   Upstream branch, if set
++ Ahead count, if upstream present
+-Behind count, if upstream present
+
+
+A series of lines are then displayed for the tracked entries.
+Ordinary changed entries have the following format; the first
+character is a 'c' to distinguish them from unmerged entries.


It would be nice (though not necessary) to have an example, either
here or at the end.


I'll try to fit that in at the bottom.


+
+cR [\t]
+
+Field   Meaning
+
+A 2 character field containing the staged and
+unstaged XY values described in the short format,
+with unchanged indicated by a "." rather than
+a space.
+   A 4 character field describing the submodule state.
+"N..." when the entry is not a submodule.
+"S" when the entry is a submodule.
+ is "C" if the commit changed; otherwise ".".
+ is "M" if it has tracked changes; otherwise ".".
+ is "U" if there are untracked changes; otherwise ".".
+The 6 character octal file modes for head, index,
+and worktree.


I think it might be more readable to be explicit: "for HEAD (),
index (), and worktree ()."


I'll try to clarify that.



+The head and index SHA1 values.
+R   The rename percentage score.


I assume this would be C copy detection heuristics percentage
score in case of copy detection, and B break percentage score
in case of breaking change into addition and deletion of file.
Or am I confused?


I'm updating to include rename and copied scores.  I haven't seen
anything in the code about a break percentage in this context.  I
see it listed with -B in the git-diff-* pages, but not in status.





+  The current pathname. It is C-Quoted if it contains
+special control characters.


I assume that "\t" tab character between  and  is here
to be able to not C-Quote sane filenames with internal whitespace,
isn't it?


I'm using the same quoting routines as the existing porcelain
format.  So yes, that looks to be the case.  It only converts
if there are insane characters in pathnames.




+   The original path. This is only present for staged renames.
+It is C-Quoted if necessary.


I assume that "C-Quoted if necessary" is the same as "C-Quoted if
it contains special control characters"; also: '"' quote character,
'\' backlash escape character and "\t" horizontal tab are not control
characters per se., but still need C-Quoting.  The rules are the same
as for the rest of Git, e.g. for `git diff`, isn't it?


yes, i'm using the same quoting routine.  i'll clarify.  thanks.




+
+
+Unmerged entries have the following format; the first character is
+a "u" to distinguish from ordinary changed entries.
+
+u 
+
+Field   Meaning
+
+A 2 character field describing the conflict type
+as described in the short format.
+   A 4 character field describing the submodule state
+as described above.
+The 6 character octal file modes for the stage 1,
+stage 2, stage 3, and worktree.


Errr... the pattern has only _3_ character octal modes,   .
A question: what happens during octopus merge?


+For regular entries, these are the head, index, and
+worktree modes; the fourth is zero.


This is remnant of the previous version of "v2" format, isn't it?


yes, sorry.  I missed that one.




+The stage 1, stage 2, and stage 3 SHA1 values.
+  The current pathname. It is C-Quoted if necessary.
+
+
+A series of lines are then displayed for untracked and ignored entries.
+
+ 
+
+Where  is "?" for untracked entries and "!" for ignored entries.


I assume that here also  is C-Quoted if necessary.


yes.




+
+When the `-z` option is 

Re: [PATCH v8 36/41] apply: don't print on stdout when be_silent is set

[Junio C Hamano]

Christian Couder  writes:

> This variable should prevent anything to be printed on both stderr
> and stdout.

You have to mention that skipping the entire callchain, not just the
"printing" part, is safe.  I can see numstat_patch_list() is
probably safe as it does not do any computation other than calling
printf() and write_name_quoted(), but other two are not immediately
obvious that what they compute are only used for their own printing
and there is no other side effects left to affect what happens after
this function returns.


> Signed-off-by: Christian Couder 
> ---
>  apply.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/apply.c b/apply.c
> index 1435f85..e2acc18 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -4698,13 +4698,13 @@ static int apply_patch(struct apply_state *state,
>   goto end;
>   }
>  
> - if (state->diffstat)
> + if (state->diffstat && !state->be_silent)
>   stat_patch_list(state, list);
>  
> - if (state->numstat)
> + if (state->numstat && !state->be_silent)
>   numstat_patch_list(state, list);
>  
> - if (state->summary)
> + if (state->summary && !state->be_silent)
>   summary_patch_list(list);
>  
>  end:
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

git cherry-pick conflict error message is deceptive when cherry-picking multiple commits

[Stephen Morton]

When I cherry-pick n commits and one of the first (n-1), fails, what I
should do is resolve the conflict, 'git add' it, and then run 'git
cherry-pick --continue'. However git advises me to

error: could not apply d0fb756... Commit message for test commit
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add ' or 'git rm '
hint: and commit the result with 'git commit'

I suspect this is not just a little bit deceptive but actually a bit
destructive, as doing a 'git commit' removes some of the sequencing
information. If I do a 'git commit', while there is .git/sequencer
information and 'git cherry-pick --continue' will work, there is no
indication in 'git status' that a cherry-pick is in progress.

It would be extremely easy for somebody to follow cherry-pick's hints
by running 'git commit' and think that they were done, not realizing
that there were m more commits remaining to be cherry-picked.

Would it be possible to expand the hint message to tell users to run
'git cherry-pick --continue'

This patch is *not* meant as a serious patch for submission --I'm sure
it's all wrong-- it's just a proof of concept and showing some
goodwill on my part that I'm trying to help and I'm willing to put in
some work if I can be pointed more in the right direction.

Regards,
Stephen




diff --git a/sequencer.c b/sequencer.c
index cdfac82..b8fa534 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -171,14 +171,21 @@ static void print_advice(int show_hint, struct
replay_opts *opts)

if (show_hint) {
if (opts->no_commit)
advise(_("after resolving the conflicts, mark
the corrected paths\n"
 "with 'git add ' or 'git rm '"));
-   else
-   advise(_("after resolving the conflicts, mark
the corrected paths\n"
-"with 'git add ' or 'git rm '\n"
-"and commit the result with 'git commit'"));
+   else if  (! file_exists(git_path_seq_dir())) {
+   advise(_("SCM: after resolving the conflicts,
mark the corrected paths\n"
+ "with 'git add ' or 'git rm '\n"
+ "and commit the result with 'git commit'"));
+}
+else
+{
+advise(_("SCM: after resolving the conflicts, mark the
corrected paths\n"
+ "with 'git add ' or 'git rm '\n"
+ "and continue the %s with 'git %s --continue'"),
action_name(opts), action_name(opts));
+}
}
 }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 35/41] apply: make 'be_silent' incompatible with 'apply_verbosely'

[Junio C Hamano]

Christian Couder  writes:

> It should be an error to have both be_silent and apply_verbosely set,
> so let's check that in check_apply_state().

Doesn't that suggest that we do not want to have a new be_silent
field at all?  Perhaps we used to have apply_verbosely = 
resulting in only two verbosity levels, "verbose" and "normal", and
you want to have another new one "total silence" or something?

If so perhaps it would be more appropriate to rename apply_verbosely
to apply_verbosity that is no longer a boolean?

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 34/41] apply: add 'be_silent' variable to 'struct apply_state'

[Junio C Hamano]

Christian Couder  writes:

> This variable should prevent anything to be printed on both stderr
> and stdout.

It is far more important to describe "why" this is needed than what
it does, the latter of which can be read from the patch text.

And I do not see any "why" here.  Is this "when the current caller
wanted to silence us, it spawned us in a separate process and
redirected our output to /dev/null, but we no longer can do that
because we will change the calling convention to allow direct calls
into us"?

Do we have a precedent to name a switch that we usually call "quiet"
or "silent" as "be_{silent,quiet}"?  Is there already a "silent"
nearby that records what the end-user gave us (e.g. via "--silent"
option), a new name may be needed, but if that is the motivation,
I'd probably call it something more specific, "apply_silently" or
somesuch.

> Let's not take care of stdout and apply_verbosely for now though,
> as that will be taken care of in following patches.
>
> Signed-off-by: Christian Couder 
> ---
>  apply.c | 43 +--
>  apply.h |  1 +
>  2 files changed, 30 insertions(+), 14 deletions(-)
>
> diff --git a/apply.c b/apply.c
> index 7bf12a7..802fa79 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -1617,8 +1617,9 @@ static void record_ws_error(struct apply_state *state,
>   return;
>  
>   err = whitespace_error_string(result);
> - fprintf(stderr, "%s:%d: %s.\n%.*s\n",
> - state->patch_input_file, linenr, err, len, line);
> + if (!state->be_silent)
> + fprintf(stderr, "%s:%d: %s.\n%.*s\n",
> + state->patch_input_file, linenr, err, len, line);
>   free(err);
>  }
>  
> @@ -1813,7 +1814,7 @@ static int parse_single_patch(struct apply_state *state,
>   return error(_("new file %s depends on old contents"), 
> patch->new_name);
>   if (0 < patch->is_delete && newlines)
>   return error(_("deleted file %s still has contents"), 
> patch->old_name);
> - if (!patch->is_delete && !newlines && context)
> + if (!patch->is_delete && !newlines && context && !state->be_silent)
>   fprintf_ln(stderr,
>  _("** warning: "
>"file %s becomes empty but is not deleted"),
> @@ -3038,8 +3039,8 @@ static int apply_one_fragment(struct apply_state *state,
>* Warn if it was necessary to reduce the number
>* of context lines.
>*/
> - if ((leading != frag->leading) ||
> - (trailing != frag->trailing))
> + if ((leading != frag->leading ||
> +  trailing != frag->trailing) && !state->be_silent)
>   fprintf_ln(stderr, _("Context reduced to (%ld/%ld)"
>" to apply fragment at %d"),
>  leading, trailing, applied_pos+1);
> @@ -3536,7 +3537,8 @@ static int try_threeway(struct apply_state *state,
>read_blob_object(, pre_sha1, patch->old_mode))
>   return error("repository lacks the necessary blob to fall back 
> on 3-way merge.");
>  
> - fprintf(stderr, "Falling back to three-way merge...\n");
> + if (!state->be_silent)
> + fprintf(stderr, "Falling back to three-way merge...\n");
>  
>   img = strbuf_detach(, );
>   prepare_image(_image, img, len, 1);
> @@ -3566,7 +3568,9 @@ static int try_threeway(struct apply_state *state,
>   status = three_way_merge(image, patch->new_name,
>pre_sha1, our_sha1, post_sha1);
>   if (status < 0) {
> - fprintf(stderr, "Failed to fall back on three-way merge...\n");
> + if (!state->be_silent)
> + fprintf(stderr,
> + "Failed to fall back on three-way merge...\n");
>   return status;
>   }
>  
> @@ -3578,9 +3582,15 @@ static int try_threeway(struct apply_state *state,
>   hashcpy(patch->threeway_stage[0].hash, pre_sha1);
>   hashcpy(patch->threeway_stage[1].hash, our_sha1);
>   hashcpy(patch->threeway_stage[2].hash, post_sha1);
> - fprintf(stderr, "Applied patch to '%s' with conflicts.\n", 
> patch->new_name);
> + if (!state->be_silent)
> + fprintf(stderr,
> + "Applied patch to '%s' with conflicts.\n",
> + patch->new_name);
>   } else {
> - fprintf(stderr, "Applied patch to '%s' cleanly.\n", 
> patch->new_name);
> + if (!state->be_silent)
> + fprintf(stderr,
> + "Applied patch to '%s' cleanly.\n",
> + patch->new_name);
>   }
>   return 0;
>  }
> @@ -4483,7 +4493,8 @@ static int write_out_one_reject(struct apply_state 
> 

Re: [PATCH v8 32/41] environment: add set_index_file()

[Junio C Hamano]

Christian Couder  writes:

> Introduce set_index_file() to be able to temporarily change the index file.
>
> It should be used like this:
>
> /* Save current index file */
> old_index_file = get_index_file();
> set_index_file((char *)tmp_index_file);
>
> /* Do stuff that will use tmp_index_file as the index file */
> ...
>
> /* When finished reset the index file */
> set_index_file(old_index_file);

WHY is glaringly missing.

> Signed-off-by: Christian Couder 
> ---

Doesn't calling this have a global effect that is flowned upon when
used by a library-ish function?  Who is the expected caller in this
series that wants to "libify" a part of the system?

>  cache.h   |  1 +
>  environment.c | 10 ++
>  2 files changed, 11 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index c73becb..8854365 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -461,6 +461,7 @@ extern int is_inside_work_tree(void);
>  extern const char *get_git_dir(void);
>  extern const char *get_git_common_dir(void);
>  extern char *get_object_directory(void);
> +extern void set_index_file(char *index_file);
>  extern char *get_index_file(void);
>  extern char *get_graft_file(void);
>  extern int set_git_dir(const char *path);
> diff --git a/environment.c b/environment.c
> index ca72464..7a53799 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -292,6 +292,16 @@ int odb_pack_keep(char *name, size_t namesz, const 
> unsigned char *sha1)
>   return open(name, O_RDWR|O_CREAT|O_EXCL, 0600);
>  }
>  
> +/*
> + * Temporarily change the index file.
> + * Please save the current index file using get_index_file() before changing
> + * the index file. And when finished, reset it to the saved value.
> + */
> +void set_index_file(char *index_file)
> +{
> + git_index_file = index_file;
> +}
> +
>  char *get_index_file(void)
>  {
>   if (!git_index_file)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/6] date: document and test "raw-local" mode

[Jeff King]

On Sat, Jul 23, 2016 at 12:15:37PM +0200, Jakub Narębski wrote:

> > diff --git a/Documentation/rev-list-options.txt 
> > b/Documentation/rev-list-options.txt
> > index 5d1de06..3ec75d4 100644
> > --- a/Documentation/rev-list-options.txt
> > +++ b/Documentation/rev-list-options.txt
> > @@ -725,8 +725,8 @@ include::pretty-options.txt[]
> > `iso-local`), the user's local time zone is used instead.
> >  +
> >  `--date=relative` shows dates relative to the current time,
> > -e.g. ``2 hours ago''. The `-local` option cannot be used with
> > -`--raw` or `--relative`.
> > +e.g. ``2 hours ago''. The `-local` option has no effect for
> > +`--relative`.
> 
> Do I understand it correctly: --relative is a short form for more
> generic --date=relative (which probably should be spelled 
> --date-format=relative), and that --date=relative-local is the
> same as --date=relative, that is *-local suffix does not change
> how date is formatted?
> 
> Because I don't think you can say --relative-local ("The `-local`
> option has no effect on `--relative`"), can you?

All correct. There is no --relative-local because "--relative" is a
historical artifact. We could support --foo for every --date=foo, but I
don't think there is a reason to do so (and reasons not to, like
avoiding cluttering the option space).

> > -`--date=raw` shows the date in the internal raw Git format `%s %z` format.
> > +`--date=raw` shows the date in the internal raw Git format `%s %z`
> > +format. Note that the `-local` option does not affect the
> > +seconds-since-epoch value (which is always measured in UTC), but does
> > +switch the accompanying timezone value.
> 
> Which is correct, logical, and next to useless, I think.

This was discussed in the earlier review. It's basically only useful if
you are feeding the output to another script which will format the date
and want to change what that script shows.

> BTW. one kind of format that Git does not support (I think) is the
> varying kind, where the precision changes with the distance from now,
> so that we can get most precision in limited width.  That's what
> `ls --long` does:
> 
>  * 'Jun 29 16:47' for dates falling in current year (more precision)
>  * 'Nov 23  2015' for dates outside (less precision, same width)
> 
> Many other programs switch from relative to absolute time when date
> in question is far in the past that relative is not very good.

Yes, this was discussed on the list not too long ago. I think it would
be useful, but is obviously orthogonal to this series.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] A Change to Commit IDs Too Ridiculous to Consider?

[Philip Oakley]

From: "Jon Forrest" 

On 7/24/2016 11:51 AM, Rodrigo Campos wrote:

And what is the problem with that, if you are doing it with instructional
purposes? Let's assume that this helps and not confuses later when the 
commits

*do* change. What is the problem you face?


A lot of instructional material contains stuff like "Do [xxx] and you'll
see [zzz]. If you don't then something went wrong so try to figure out
what happened and do it again."

Git, as it stands, for good reason doesn't allow this approach.


You may want to look at how the test suite handles the need for well defined 
commit sequences.


It's not something I've really studied, but I am aware of the test_tick to 
increment the time and similar helpers.


There is a big learning step that needs to be got over by many beginners who 
have no concept of a DVCS, nor of multiple master copies (which to most is 
an oxymoron!), nor why the sha is a good solution and serial numbers are a 
bad solution!.


Being able to do a few "Hello World" commits starting at unix t=0, and then 
progressing on to see how they differ when it's unix=now time, or they use 
their own user IDs could be a useful step for those that need it.




I don't think a Git beginner, when using a version of Git that somehow
works the way I proposed, will be confused. The fact that performing the
same steps results in the same commit IDs won't be something that
they'll care about or even notice. The material can include a callout
mentioning the difference between "real" Git and "learners" Git.

I mean, for some examples you can use HEAD, HEAD^, HEAD~4, etc. and that 
always

works, no matter the commit id.


This will work in some cases, but should come later in a Git book.
But, in many cases using relative commit IDs, rather than absolute,
will be less clear (I believe).


In which cases do you want/need the commit ids to be equal?
Can you be more specific?


Sure. Take a look at the 2nd or 3rd chapter of Pro Git Reedited, 2nd
Edition (or just Pro Git 2nd Edition - it doesn't matter). You see
lots of output showing 'git commit' commands and the commit IDs that
result. I suspect you'd see the same in almost any book about Git.

Jon
--
Philip 


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/4] submodule: support running in multiple worktree setup

[Stefan Beller]

On Tue, Jul 26, 2016 at 10:20 AM, Duy Nguyen  wrote:
> On Tue, Jul 26, 2016 at 1:25 AM, Stefan Beller  wrote:
>> So what is the design philosophy in worktrees? How much independence does
>> one working tree have?
>
> git-worktree started out as an alternative for git-stash: hmm.. i need
> to make some changes in another branch, okay let's leave this worktree
> (with all its messy stuff) as-is, create another worktree, make those
> changes, then delete the worktree and go back here. There's already
> another way of doing that without git-stash: you clone the repo, fix
> your stuff, push back and delete the new repo.
>
> I know I have not really answered your questions. But I think it gives
> an idea what are the typical use cases for multiple worktrees. How
> much independence would need to be decided case-by-case, I think.

Thanks!


>
>> So here is what I did:
>>  *  s/git submodule init/git submodule update --init/
>>  * added a test_pause to the last test on the last line
>>  * Then:
>>
>> $ find . |grep da5e6058
>> ./addtest/.git/modules/submod/objects/08/da5e6058267d6be703ae058d173ce38ed53066
>> ./addtest/.git/worktrees/super-elsewhere/modules/submod/objects/08/da5e6058267d6be703ae058d173ce38ed53066
>> ./addtest/.git/worktrees/super-elsewhere/modules/submod2/objects/08/da5e6058267d6be703ae058d173ce38ed53066
>> ./.git/objects/08/da5e6058267d6be703ae058d173ce38ed53066
>>
>> The last entry is the "upstream" for the addtest clone, so that is fine.
>> However inside the ./addtest/ (and its worktrees, which currently are
>> embedded in there?) we only want to have one object store for a given
>> submodule?
>
> How to store stuff in .git is the implementation details that the user
> does not care about.

They do unfortunately. :(
Some teams here are trying to migrate from the repo[1] tool to submodules,
and they usually have large code bases. (e.g. The Android Open Source
Project[2], put into a superproject has a .git dir size of 17G. The
17G are partitioned as follows:

.../.git$ du --max-depth=1 -h
44K ./hooks
32K ./refs
36K ./logs
17G ./modules
4.0K ./branches
8.0K ./info
4.7M ./objects
17G .

i.e. roughly all in submodules.

So our users do care about both what is on disk, as well
as what goes over the wire (network traffic).

My sudden interest in worktrees came up when I learned the
`--reference` flag for submodule operations is broken for
our use case, and instead of fixing the `--reference` flag,
I think the worktree approach is generally saner (i.e. with the
references you may have nasty gc issues IIUC, but in the
worktree world gc knows about all the working trees, detached
heads and branches.)

[1] https://source.android.com/source/developing.html
[2] https://android.googlesource.com/

> As long as we keep the behavior the same (they
> can still "git submodule init" and stuff in the new worktree), sharing
> the same object store makes sense (pros: lower disk consumption, cons:
> none).

So I think the current workflow for submodules
may need some redesign anyway as the submodule
commands were designed with a strict "one working
tree only" assumption.

Submodule URLs  are stored in 3 places:
 A) In the .gitmodules file of the superproject
 B) In the option submodule..URL in the superproject
 C) In the remote.origin.URL in the submodule

A) is a recommendation from the superproject to make life
of downstream easier to find and setup the whole thing.
You can ignore that if you want, though generally a caring
upstream provides good URLs here.

C) is where we actually fetch from (and hope it has all
the sha1s that are recorded as gitlinks in the superproejct)

B) seems like a hack to enable the workflow as below:

Current workflow for handling submodule URLs:
 1) Clone the superproject
 2) Run git submodule init on desired submodules
 3) Inspect .git/config to see if any submodule URL needs adaption
 4) Run git submodule update to obtain the submodules from
the configured place
 5) In case of superproject adapting the URL
-> git submodule sync, which overwrites the submodule..URL in the
superprojects .git/config as well as configuring the
remote."$remote".url in the submodule
 6) In case of users desire to change the URL
-> No one command to solve it; possible workaround: edit
.gitmodules and git submodule sync, or configure  the submodule..URL
in the superprojects .git/config as well as configuring the
remote."$remote".url in
the submodule separately. Although just changing the submodules remote works
just as well (until you remove and re-clone the submodule)

One could imagine another workflow:
 1) clone the superproject, which creates empty repositories for the
submodules
 (2) from the prior workflow is gone
 3) instead of inspecting .git/config you can directly manipulate the
remote.$remote.url configuration in the submodule.
 4) Run git submodule update to obtain the submodules from

Re: [PATCH v10 12/12] bisect--helper: `get_terms` & `bisect_terms` shell function in C

[Junio C Hamano]

Torsten Bögershausen  writes:

> On 07/25/2016 06:53 PM, Junio C Hamano wrote:
>> Pranit Bauva  writes:
>>
> >>> +enum terms_defined {
> >>> +   TERM_BAD = 1,
> >>> +   TERM_GOOD = 2,
> >>> +   TERM_NEW = 4,
> >>> +   TERM_OLD = 8
> >>> +};
> >>> +
 >> ...
> Is there any risk that a more generic term like "TERM_BAD" may collide
> with some other definition some day ?
>
> Would it make sense to call it GIT_BISECT_TERM_BAD, GBS_TERM_BAD,
> BIS_TERM_BAD or something more unique ?

I am not sure if the scope of these symbols would ever escape
outside bisect-helper.c (and builtin/bisect.c eventually when we
retire git-bisect.sh), but BISECT_TERM_{GOOD,BAD,OLD,NEW} would not
be too bad.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] submodule-config: combine error checking if clauses

[Stefan Beller]

On Tue, Jul 26, 2016 at 2:49 AM, Heiko Voigt  wrote:
> So we have less return handling code.
>
> Signed-off-by: Heiko Voigt 

Thanks,
Stefan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] fix passing a name for config from submodules

[Stefan Beller]

On Tue, Jul 26, 2016 at 2:49 AM, Heiko Voigt  wrote:

Thanks for continuing on the submodule cache!

> In commit 959b5455 we implemented the initial version of the submodule

Usually we refer to the commit by a triple of "abbrev. sha1 (date, subject).
See d201a1ecd (2015-05-21, test_bitmap_walk: free bitmap with bitmap_free)
for an example. Or ce41720ca (2015-04-02, blame, log: format usage strings
similarly to those in documentation).

Apparently we put the subject first and then the date. I always did it
the other way
round, to there is no strict coding guide line, though it helps a lot to have an
understanding for a) how long are we in the "broken" state already as well as
b) what was the rationale for introducing it.



> @@ -397,8 +397,10 @@ static const struct submodule *config_from(struct 
> submodule_cache *cache,
> return entry->config;
> }
>
> -   if (!gitmodule_sha1_from_commit(commit_sha1, sha1))
> +   if (!gitmodule_sha1_from_commit(commit_sha1, sha1, )) {
> +   strbuf_release();
> return NULL;

This is a reoccuring pattern below. Maybe it might make sense to
just do a s/return.../ goto out/ and at that label we cleanup `rev` and `config`
and return a result value?
There are currently 6 early returns (not counting the 3 from the last switch),
4 of them return NULL, so that would result in just a "goto out", whereas 2
return an actual value, they would need to assign the result value first before
jumping out of the logic. I dunno, just food for though.



> @@ -425,8 +432,9 @@ static const struct submodule *config_from(struct 
> submodule_cache *cache,
> parameter.commit_sha1 = commit_sha1;
> parameter.gitmodules_sha1 = sha1;
> parameter.overwrite = 0;
> -   git_config_from_mem(parse_config, "submodule-blob", "",
> +   git_config_from_mem(parse_config, "submodule-blob", rev.buf,
> config, config_size, );

Ok, this is the actual fix. Do you want to demonstrate its impact by adding
one or two tests that failed before and now work?
(As I was using the submodule config API most of the time with null_sha1
to indicate we'd be looking at the current .gitmodules file in the worktree,
the actual bug may have not manifested in the users of this API.
But still, it would be nice to see what was broken?)

Thanks,
Stefan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/4] submodule: support running in multiple worktree setup

[Duy Nguyen]

On Tue, Jul 26, 2016 at 1:25 AM, Stefan Beller  wrote:
> So what is the design philosophy in worktrees? How much independence does
> one working tree have?

git-worktree started out as an alternative for git-stash: hmm.. i need
to make some changes in another branch, okay let's leave this worktree
(with all its messy stuff) as-is, create another worktree, make those
changes, then delete the worktree and go back here. There's already
another way of doing that without git-stash: you clone the repo, fix
your stuff, push back and delete the new repo.

I know I have not really answered your questions. But I think it gives
an idea what are the typical use cases for multiple worktrees. How
much independence would need to be decided case-by-case, I think.

> So here is what I did:
>  *  s/git submodule init/git submodule update --init/
>  * added a test_pause to the last test on the last line
>  * Then:
>
> $ find . |grep da5e6058
> ./addtest/.git/modules/submod/objects/08/da5e6058267d6be703ae058d173ce38ed53066
> ./addtest/.git/worktrees/super-elsewhere/modules/submod/objects/08/da5e6058267d6be703ae058d173ce38ed53066
> ./addtest/.git/worktrees/super-elsewhere/modules/submod2/objects/08/da5e6058267d6be703ae058d173ce38ed53066
> ./.git/objects/08/da5e6058267d6be703ae058d173ce38ed53066
>
> The last entry is the "upstream" for the addtest clone, so that is fine.
> However inside the ./addtest/ (and its worktrees, which currently are
> embedded in there?) we only want to have one object store for a given
> submodule?

How to store stuff in .git is the implementation details that the user
does not care about. As long as we keep the behavior the same (they
can still "git submodule init" and stuff in the new worktree), sharing
the same object store makes sense (pros: lower disk consumption, cons:
none).


> After playing with this series a bit more, I actually like the UI as it is an
> easy mental model "submodules behave completely independent".
>
> However in 3/4 you said:
>
> + - `submodule.*` in current state should not be shared because the
> +   information is tied to a particular version of .gitmodules in a
> +   working directory.
>
> This is already a problem with say different branches/versions.
> That has been solved by duplicating that information to .git/config
> as a required step. (I don't like that approach, as it is super confusing
> IMHO)

Hmm.. I didn't realize this. But then I have never given much thought
about submodules, probably because I have an alternative solution for
it (or some of its use cases) anyway :)

OK so it's already a problem. But if we keep sharing submodule stuff
in .git/config, there's a _new_ problem: when you "submodule init" a
worktree, .git/config is now tailored for the current worktree, when
you move back to the previous worktree, you need to "submodule init"
again. So moving to multiple worktrees setup changes how the user uses
submodule, not good in my opinion.

If you have a grand plan to make submodule work at switching branches
(without reinit) and if it happens to work the same way when we have
multiple worktrees, great.

> I am back to the drawing board for the submodule side of things,
> but I guess this series could be used once we figure out how to
> have just one object database for a submodule.

I would leave this out for now. Let's make submodule work with
multiple worktrees first (and see how the users react to this). Then
we can try to share object database. Object database and refs are tied
closely together so you may run into other problems soon.
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 11/16] am -3: use merge_recursive() directly again

[Junio C Hamano]

Johannes Schindelin  writes:

> If you want, I can break out the subsequent patches into a separate
> series.

I do not think that would help anybody, as we'll have to review them
anyway.  If some of the the later ones were "oops, this earlier step
did an incomplete job and here is a fix-up", then squashing them
into the step where such a mistake happens may reduce the review
load, but I suspect that is not the case (iow, they are enhancements
and the earlier ones can stand on their own).

> I may have missed something as stupid as an unclosed file handle, after
> all.

Yes, reviewing the same change over and over, especially if the
change is your own, tends to have diminishing rate of bug yields,
which is why they need to be reviewed by fresh set of eyes.



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] i18n: notes: mark comment for translation

[Junio C Hamano]

Vasco Almeida  writes:

> A Seg, 25-07-2016 às 10:49 -0700, Junio C Hamano escreveu:
>> Vasco Almeida  writes:
>> 
>> > 
>> >  static const char note_template[] =
>> > -  "\nWrite/edit the notes for the following object:\n";
>> > +  N_("Write/edit the notes for the following object:");
>> 
>> I do not quite understand why you want the blank lines surrounding
>> the message outside add_commented_lines() call.  I think the intent
>> is to produce
>> 
>> #
>> # Write/edit the notes for the following object:
>> #
>
> Yes, this was my intention. The original does:
>
>     #
>     # Write/edit the notes for the following object:
>     

Ah, of course, I misspoke.  The original "\n\n" requests
that an empty line that is commented, followed by a line with the
message that is also commented, is given at that point.  The last
commented blank was my mistake.

In any case, I do not understand why you want to exclude the LFs
from the message.  If a translation for a particular language is
very long and would not fit on a single line, the translator is
allowed to make the message much longer, i.e. the translated version
of the  part may contain one or more LFs (which is what the
add-commented-lines function was invented for).  I'd think having
LFs in the to-be-translated-original will serve as a good hint to
signal translators that is the case.

Thansk.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] i18n: notes: mark comment for translation

[Junio C Hamano]

Vasco Almeida  writes:

> + strbuf_add_commented_lines(, "\n", strlen("\n"));
> + strbuf_add_commented_lines(, _(note_template), 
> strlen(_(note_template)));
> + strbuf_add_commented_lines(, "\n", strlen("\n"));

Hmm, do we really need to make three separate calls?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] t5510: skip tests under GETTEXT_POISON build

[Junio C Hamano]

Vasco Almeida  writes:

> Skip tests when running under GETTEXT_POISON build and run them with
> C_LOCALE_OUTPUT prerequisite.
>
> These tests are irrelevant under GETTEXT_POISON because they test text
> output alignment which GETTEXT_POISON turns useless.
>
> Signed-off-by: Vasco Almeida 
> ---
>  t/t5510-fetch.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index 6bd4853..668c54b 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -688,7 +688,7 @@ test_expect_success 'fetching with auto-gc does not lock 
> up' '
>   )
>  '
>  
> -test_expect_success 'fetch aligned output' '
> +test_expect_success C_LOCALE_OUTPUT 'fetch aligned output' '
>   git clone . full-output &&
>   test_commit long-tag &&
>   (
> @@ -703,7 +703,7 @@ test_expect_success 'fetch aligned output' '
>   test_cmp expect actual
>  '
>  
> -test_expect_success 'fetch compact output' '
> +test_expect_success C_LOCALE_OUTPUT 'fetch compact output' '
>   git clone . compact &&
>   test_commit extraaa &&
>   (

Makes sense, will queue.

This is a tangent, but it may make sense for us to start thinking
about retiring one of the two prerequisites, GETTEXT_POISON and
C_LOCALE_OUTPUT.  Back when 5e9637c6 (i18n: add infrastructure for
translating Git with gettext, 2011-11-18) introduced the former,
test_have_prereq did not support a negated prerequisite, so the
commit added GETTEXT_POISON prerequisite; if we had the modern
test_have_prereq, we would have written

test_expect_success GETTEXT_POISON '...'

that appear in t0205 as

test_expect_success !C_LOCALE_OUTPUT '...'

I would think.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 16/16] merge-recursive: flush output buffer even when erroring out

[Johannes Schindelin]

Ever since 66a155b (Enable output buffering in merge-recursive.,
2007-01-14), we had a problem: When the merge failed in a fatal way, all
regular output was swallowed because we called die() and did not get a
chance to drain the output buffers.

To fix this, several modifications were necessary:

- we needed to stop die()ing, to give callers a chance to do something
  when an error occurred (in this case, flush the output buffers),

- we needed to delay printing the error message so that the caller can
  print the buffered output before that, and

- we needed to make sure that the output buffers are flushed even when
  the return value indicates an error.

The first two changes were introduced through earlier commits in this
patch series, and this commit addresses the third one.

Signed-off-by: Johannes Schindelin 
---
 merge-recursive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index a16b150..66e93e0 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2069,6 +2069,7 @@ int merge_recursive(struct merge_options *o,
o->ancestor = "merged common ancestors";
clean = merge_trees(o, h1->tree, h2->tree, 
merged_common_ancestors->tree,
);
+   flush_output(o);
if (clean < 0)
return clean;
 
@@ -2077,7 +2078,6 @@ int merge_recursive(struct merge_options *o,
commit_list_insert(h1, &(*result)->parents);
commit_list_insert(h2, &(*result)->parents->next);
}
-   flush_output(o);
if (o->buffer_output < 2)
strbuf_release(>obuf);
if (show(o, 2))
-- 
2.9.0.281.g286a8d9
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 13/16] merge-recursive: write the commit title in one go

[Johannes Schindelin]

In 66a155b (Enable output buffering in merge-recursive., 2007-01-14), we
changed the code such that it prints the output in one go, to avoid
interfering with the progress output.

Let's make sure that the same holds true when outputting the commit
title: previously, we used several printf() statements to stdout and
speculated that stdout's buffer is large enough to hold the entire
commit title.

Apart from making that speculation unnecessary, we change the code to
add the message to the output buffer before flushing for another reason:
the next commit will introduce a new level of output buffering, where
the caller can request the output not to be flushed, but to be retained
for further processing.

This latter feature will be needed when teaching the sequencer to do
rebase -i's brunt work: it wants to control the output of the
cherry-picks (i.e. recursive merges).

Signed-off-by: Johannes Schindelin 
---
 merge-recursive.c | 17 +
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 71a0aa0..723b8d0 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -191,25 +191,26 @@ static void output(struct merge_options *o, int v, const 
char *fmt, ...)
 
 static void output_commit_title(struct merge_options *o, struct commit *commit)
 {
-   int i;
-   flush_output(o);
-   for (i = o->call_depth; i--;)
-   fputs("  ", stdout);
+   strbuf_addchars(>obuf, ' ', o->call_depth * 2);
if (commit->util)
-   printf("virtual %s\n", merge_remote_util(commit)->name);
+   strbuf_addf(>obuf, "virtual %s\n",
+   merge_remote_util(commit)->name);
else {
-   printf("%s ", find_unique_abbrev(commit->object.oid.hash, 
DEFAULT_ABBREV));
+   strbuf_addf(>obuf, "%s ",
+   find_unique_abbrev(commit->object.oid.hash,
+   DEFAULT_ABBREV));
if (parse_commit(commit) != 0)
-   printf(_("(bad commit)\n"));
+   strbuf_addf(>obuf, _("(bad commit)\n"));
else {
const char *title;
const char *msg = get_commit_buffer(commit, NULL);
int len = find_commit_subject(msg, );
if (len)
-   printf("%.*s\n", len, title);
+   strbuf_addf(>obuf, "%.*s\n", len, title);
unuse_commit_buffer(commit, msg);
}
}
+   flush_output(o);
 }
 
 static int add_cacheinfo(struct merge_options *o,
-- 
2.9.0.281.g286a8d9


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 11/16] am -3: use merge_recursive() directly again

[Johannes Schindelin]

Last October, we had to change this code to run `git merge-recursive`
in a child process: git-am wants to print some helpful advice when the
merge failed, but the code in question was not prepared to return, it
die()d instead.

We are finally at a point when the code *is* prepared to return errors,
and can avoid the child process again.

This reverts commit c63d4b2 (am -3: do not let failed merge from
completing the error codepath, 2015-10-09), with the necessary changes
to adjust for the fact that Git's source code changed in the meantime
(such as: using OIDs instead of hashes in the recursive merge, and a
removed gender bias).

Note: the code now calls merge_recursive_generic() again. Unlike
merge_trees() and merge_recursive(), this function returns 0 upon success,
as most of Git's functions. Therefore, the error value -1 naturally is
handled correctly, and we do not have to take care of it specifically.

Signed-off-by: Johannes Schindelin 
---
 builtin/am.c | 62 +---
 1 file changed, 22 insertions(+), 40 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index b77bf11..cfb79ea 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1579,47 +1579,18 @@ static int build_fake_ancestor(const struct am_state 
*state, const char *index_f
 }
 
 /**
- * Do the three-way merge using fake ancestor, their tree constructed
- * from the fake ancestor and the postimage of the patch, and our
- * state.
- */
-static int run_fallback_merge_recursive(const struct am_state *state,
-   unsigned char *orig_tree,
-   unsigned char *our_tree,
-   unsigned char *their_tree)
-{
-   struct child_process cp = CHILD_PROCESS_INIT;
-   int status;
-
-   cp.git_cmd = 1;
-
-   argv_array_pushf(_array, "GITHEAD_%s=%.*s",
-sha1_to_hex(their_tree), linelen(state->msg), 
state->msg);
-   if (state->quiet)
-   argv_array_push(_array, "GIT_MERGE_VERBOSITY=0");
-
-   argv_array_push(, "merge-recursive");
-   argv_array_push(, sha1_to_hex(orig_tree));
-   argv_array_push(, "--");
-   argv_array_push(, sha1_to_hex(our_tree));
-   argv_array_push(, sha1_to_hex(their_tree));
-
-   status = run_command() ? (-1) : 0;
-   discard_cache();
-   read_cache();
-   return status;
-}
-
-/**
  * Attempt a threeway merge, using index_path as the temporary index.
  */
 static int fall_back_threeway(const struct am_state *state, const char 
*index_path)
 {
-   unsigned char orig_tree[GIT_SHA1_RAWSZ], their_tree[GIT_SHA1_RAWSZ],
- our_tree[GIT_SHA1_RAWSZ];
+   struct object_id orig_tree, their_tree, our_tree;
+   const struct object_id *bases[1] = { _tree };
+   struct merge_options o;
+   struct commit *result;
+   char *their_tree_name;
 
-   if (get_sha1("HEAD", our_tree) < 0)
-   hashcpy(our_tree, EMPTY_TREE_SHA1_BIN);
+   if (get_oid("HEAD", _tree) < 0)
+   hashcpy(our_tree.hash, EMPTY_TREE_SHA1_BIN);
 
if (build_fake_ancestor(state, index_path))
return error("could not build fake ancestor");
@@ -1627,7 +1598,7 @@ static int fall_back_threeway(const struct am_state 
*state, const char *index_pa
discard_cache();
read_cache_from(index_path);
 
-   if (write_index_as_tree(orig_tree, _index, index_path, 0, NULL))
+   if (write_index_as_tree(orig_tree.hash, _index, index_path, 0, 
NULL))
return error(_("Repository lacks necessary blobs to fall back 
on 3-way merge."));
 
say(state, stdout, _("Using index info to reconstruct a base tree..."));
@@ -1643,7 +1614,7 @@ static int fall_back_threeway(const struct am_state 
*state, const char *index_pa
init_revisions(_info, NULL);
rev_info.diffopt.output_format = DIFF_FORMAT_NAME_STATUS;
diff_opt_parse(_info.diffopt, _filter_str, 1, 
rev_info.prefix);
-   add_pending_sha1(_info, "HEAD", our_tree, 0);
+   add_pending_sha1(_info, "HEAD", our_tree.hash, 0);
diff_setup_done(_info.diffopt);
run_diff_index(_info, 1);
}
@@ -1652,7 +1623,7 @@ static int fall_back_threeway(const struct am_state 
*state, const char *index_pa
return error(_("Did you hand edit your patch?\n"
"It does not apply to blobs recorded in its 
index."));
 
-   if (write_index_as_tree(their_tree, _index, index_path, 0, NULL))
+   if (write_index_as_tree(their_tree.hash, _index, index_path, 0, 
NULL))
return error("could not write tree");
 
say(state, stdout, _("Falling back to patching base and 3-way 
merge..."));
@@ -1668,11 +1639,22 @@ static int fall_back_threeway(const struct am_state 
*state, const char *index_pa
 * changes.
   

[PATCH v5 10/16] merge-recursive: switch to returning errors instead of dying

[Johannes Schindelin]

The recursive merge machinery is supposed to be a library function, i.e.
it should return an error when it fails. Originally the functions were
part of the builtin "merge-recursive", though, where it was simpler to
call die() and be done with error handling.

The existing callers were already prepared to detect negative return
values to indicate errors and to behave as previously: exit with code 128
(which is the same thing that die() does, after printing the message).

Signed-off-by: Johannes Schindelin 
---
 merge-recursive.c | 62 +++
 1 file changed, 35 insertions(+), 27 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 6beb1e4..bc59815 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -275,8 +275,10 @@ struct tree *write_tree_from_memory(struct merge_options 
*o)
active_cache_tree = cache_tree();
 
if (!cache_tree_fully_valid(active_cache_tree) &&
-   cache_tree_update(_index, 0) < 0)
-   die(_("error building trees"));
+   cache_tree_update(_index, 0) < 0) {
+   error(_("error building trees"));
+   return NULL;
+   }
 
result = lookup_tree(active_cache_tree->sha1);
 
@@ -716,12 +718,10 @@ static int make_room_for_path(struct merge_options *o, 
const char *path)
/* Make sure leading directories are created */
status = safe_create_leading_directories_const(path);
if (status) {
-   if (status == SCLD_EXISTS) {
+   if (status == SCLD_EXISTS)
/* something else exists */
-   error(msg, path, _(": perhaps a D/F conflict?"));
-   return -1;
-   }
-   die(msg, path, "");
+   return error(msg, path, _(": perhaps a D/F conflict?"));
+   return error(msg, path, "");
}
 
/*
@@ -749,6 +749,8 @@ static int update_file_flags(struct merge_options *o,
 int update_cache,
 int update_wd)
 {
+   int ret = 0;
+
if (o->call_depth)
update_wd = 0;
 
@@ -769,9 +771,11 @@ static int update_file_flags(struct merge_options *o,
 
buf = read_sha1_file(oid->hash, , );
if (!buf)
-   die(_("cannot read object %s '%s'"), oid_to_hex(oid), 
path);
-   if (type != OBJ_BLOB)
-   die(_("blob expected for %s '%s'"), oid_to_hex(oid), 
path);
+   return error(_("cannot read object %s '%s'"), 
oid_to_hex(oid), path);
+   if (type != OBJ_BLOB) {
+   ret = error(_("blob expected for %s '%s'"), 
oid_to_hex(oid), path);
+   goto free_buf;
+   }
if (S_ISREG(mode)) {
struct strbuf strbuf = STRBUF_INIT;
if (convert_to_working_tree(path, buf, size, )) {
@@ -792,8 +796,11 @@ static int update_file_flags(struct merge_options *o,
else
mode = 0666;
fd = open(path, O_WRONLY | O_TRUNC | O_CREAT, mode);
-   if (fd < 0)
-   die_errno(_("failed to open '%s'"), path);
+   if (fd < 0) {
+   ret = error_errno(_("failed to open '%s'"),
+ path);
+   goto free_buf;
+   }
write_in_full(fd, buf, size);
close(fd);
} else if (S_ISLNK(mode)) {
@@ -801,18 +808,18 @@ static int update_file_flags(struct merge_options *o,
safe_create_leading_directories_const(path);
unlink(path);
if (symlink(lnk, path))
-   die_errno(_("failed to symlink '%s'"), path);
+   ret = error_errno(_("failed to symlink '%s'"), 
path);
free(lnk);
} else
-   die(_("do not know what to do with %06o %s '%s'"),
-   mode, oid_to_hex(oid), path);
+   ret = error(_("do not know what to do with %06o %s 
'%s'"),
+   mode, oid_to_hex(oid), path);
  free_buf:
free(buf);
}
  update_index:
-   if (update_cache)
+   if (!ret && update_cache)
add_cacheinfo(mode, oid, path, 0, update_wd, 
ADD_CACHE_OK_TO_ADD);
-   return 0;
+   return ret;
 }
 
 static int update_file(struct merge_options *o,
@@ -938,20 +945,22 @@ static int merge_file_1(struct merge_options *o,
oidcpy(>oid, >oid);
else if (S_ISREG(a->mode)) {
mmbuffer_t 

[PATCH v5 15/16] Ensure that the output buffer is released after calling merge_trees()

[Johannes Schindelin]

The recursive merge machinery accumulates its output in an output
buffer, to be flushed at the end of merge_recursive(). At this point,
we forgot to release the output buffer.

When calling merge_trees() (i.e. the non-recursive part of the recursive
merge) directly, the output buffer is never flushed because the caller
may be merge_recursive() which wants to flush the output itself.

For the same reason, merge_trees() cannot release the output buffer: it
may still be needed.

Forgetting to release the output buffer did not matter much when running
git-checkout, or git-merge-recursive, because we exited after the
operation anyway. Ever since cherry-pick learned to pick a commit range,
however, this memory leak had the potential of becoming a problem.

Signed-off-by: Johannes Schindelin 
---
 builtin/checkout.c | 1 +
 merge-recursive.c  | 2 ++
 sequencer.c| 1 +
 3 files changed, 4 insertions(+)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 07dea3b..8d852d4 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -573,6 +573,7 @@ static int merge_working_tree(const struct checkout_opts 
*opts,
exit(128);
ret = reset_tree(new->commit->tree, opts, 0,
 writeout_error);
+   strbuf_release();
if (ret)
return ret;
}
diff --git a/merge-recursive.c b/merge-recursive.c
index 311cfa4..a16b150 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2078,6 +2078,8 @@ int merge_recursive(struct merge_options *o,
commit_list_insert(h2, &(*result)->parents->next);
}
flush_output(o);
+   if (o->buffer_output < 2)
+   strbuf_release(>obuf);
if (show(o, 2))
diff_warn_rename_limit("merge.renamelimit",
   o->needed_rename_limit, 0);
diff --git a/sequencer.c b/sequencer.c
index 286a435..ec50519 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -293,6 +293,7 @@ static int do_recursive_merge(struct commit *base, struct 
commit *next,
clean = merge_trees(,
head_tree,
next_tree, base_tree, );
+   strbuf_release();
if (clean < 0)
return clean;
 
-- 
2.9.0.281.g286a8d9


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 12/16] merge-recursive: flush output buffer before printing error messages

[Johannes Schindelin]

The data structure passed to the recursive merge machinery has a feature
where the caller can ask for the output to be buffered into a strbuf, by
setting the field 'buffer_output'.

Previously, we simply swallowed the buffered output when showing error
messages. With this patch, we show the output first, and only then print
the error message.

Currently, the only user of that buffering is merge_recursive() itself,
to avoid the progress output to interfere.

In the next patches, we will introduce a new buffer_output mode that
forces merge_recursive() to retain the output buffer for further
processing by the caller. If the caller asked for that, we will then
also write the error messages into the output buffer. This is necessary
to give the caller more control not only how to react in case of errors
but also control how/if to display the error messages.

Signed-off-by: Johannes Schindelin 
---
 merge-recursive.c | 116 --
 1 file changed, 68 insertions(+), 48 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index bc59815..71a0aa0 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -23,6 +23,28 @@
 #include "dir.h"
 #include "submodule.h"
 
+static void flush_output(struct merge_options *o)
+{
+   if (o->obuf.len) {
+   fputs(o->obuf.buf, stdout);
+   strbuf_reset(>obuf);
+   }
+}
+
+static int err(struct merge_options *o, const char *err, ...)
+{
+   va_list params;
+
+   va_start(params, err);
+   flush_output(o);
+   strbuf_vaddf(>obuf, err, params);
+   error("%s", o->obuf.buf);
+   strbuf_reset(>obuf);
+   va_end(params);
+
+   return -1;
+}
+
 static struct tree *shift_tree_object(struct tree *one, struct tree *two,
  const char *subtree_shift)
 {
@@ -148,14 +170,6 @@ static int show(struct merge_options *o, int v)
return (!o->call_depth && o->verbosity >= v) || o->verbosity >= 5;
 }
 
-static void flush_output(struct merge_options *o)
-{
-   if (o->obuf.len) {
-   fputs(o->obuf.buf, stdout);
-   strbuf_reset(>obuf);
-   }
-}
-
 __attribute__((format (printf, 3, 4)))
 static void output(struct merge_options *o, int v, const char *fmt, ...)
 {
@@ -198,7 +212,8 @@ static void output_commit_title(struct merge_options *o, 
struct commit *commit)
}
 }
 
-static int add_cacheinfo(unsigned int mode, const struct object_id *oid,
+static int add_cacheinfo(struct merge_options *o,
+   unsigned int mode, const struct object_id *oid,
const char *path, int stage, int refresh, int options)
 {
struct cache_entry *ce;
@@ -206,7 +221,7 @@ static int add_cacheinfo(unsigned int mode, const struct 
object_id *oid,
 
ce = make_cache_entry(mode, oid ? oid->hash : null_sha1, path, stage, 
0);
if (!ce)
-   return error(_("addinfo_cache failed for path '%s'"), path);
+   return err(o, _("addinfo_cache failed for path '%s'"), path);
 
ret = add_cache_entry(ce, options);
if (refresh) {
@@ -276,7 +291,7 @@ struct tree *write_tree_from_memory(struct merge_options *o)
 
if (!cache_tree_fully_valid(active_cache_tree) &&
cache_tree_update(_index, 0) < 0) {
-   error(_("error building trees"));
+   err(o, _("error building trees"));
return NULL;
}
 
@@ -544,7 +559,8 @@ static struct string_list *get_renames(struct merge_options 
*o,
return renames;
 }
 
-static int update_stages(const char *path, const struct diff_filespec *o,
+static int update_stages(struct merge_options *opt, const char *path,
+const struct diff_filespec *o,
 const struct diff_filespec *a,
 const struct diff_filespec *b)
 {
@@ -563,13 +579,13 @@ static int update_stages(const char *path, const struct 
diff_filespec *o,
if (remove_file_from_cache(path))
return -1;
if (o)
-   if (add_cacheinfo(o->mode, >oid, path, 1, 0, options))
+   if (add_cacheinfo(opt, o->mode, >oid, path, 1, 0, options))
return -1;
if (a)
-   if (add_cacheinfo(a->mode, >oid, path, 2, 0, options))
+   if (add_cacheinfo(opt, a->mode, >oid, path, 2, 0, options))
return -1;
if (b)
-   if (add_cacheinfo(b->mode, >oid, path, 3, 0, options))
+   if (add_cacheinfo(opt, b->mode, >oid, path, 3, 0, options))
return -1;
return 0;
 }
@@ -720,8 +736,8 @@ static int make_room_for_path(struct merge_options *o, 
const char *path)
if (status) {
if (status == SCLD_EXISTS)
/* something else exists */
-   return error(msg, path, _(": perhaps a D/F 

[PATCH v5 00/16] Use merge_recursive() directly in the builtin am

[Johannes Schindelin]

This is the fifth iteration of the long-awaited re-roll of the attempt to
avoid spawning merge-recursive from the builtin am and use merge_recursive()
directly instead.

The *real* reason for the reroll is that I need a libified recursive
merge to accelerate the interactive rebase by teaching the sequencer to
do rebase -i's grunt work. Coming with a very nice 3x-5x speedup of
`rebase -i`.

In this endeavor, we need to be extra careful to retain backwards
compatibility. The test script t6022-merge-rename.sh, for example, verifies
that `git pull` exits with status 128 in case of a fatal error. To that end,
we need to make sure that fatal errors are handled by existing (builtin)
users via exit(128) (or die(), which calls exit(128) at the end).  New users
(such as a builtin helper doing rebase -i's grunt work) may want to print
some helpful advice what happened and how to get out of this mess before
erroring out.

The changes relative to the fourth iteration of this patch series:

- the first patch which introduces a tests case to t5520 was prettified:

  - it uses test_seq now,
  - it avoids `printf` when `echo` does the job, too,
  - it adds a missing empty line between test cases, and
  - it clarifies why we need to check out with force.

- the change that would have made the bug report about a too-small
  buffer in imap-send was reverted, because it did more than was claimed
  by the commit message.

- the "Let's teach them manners" part of one commit message was replaced
  with a less flippant description.

- the comment before merge_recursive() that claims that the return value
  is ignored now clarifies that it is ignored unless it indicates an
  error.

- during the rebase to `master`, a trivial merge conflict with the
  `jc/renormalize-merge-kill-safer-crlf` branch was resolved.

This patch series touches rather important code. Now that I addressed
concerns such as prettifying some test code, I would appreciate thorough
reviews with a focus on the critical parts of the code, those that could
result in regressions.


Johannes Schindelin (16):
  t5520: verify that `pull --rebase` shows the helpful advice when
failing
  Report bugs consistently
  Avoid translating bug messages
  merge-recursive: clarify code in was_tracked()
  Prepare the builtins for a libified merge_recursive()
  merge_recursive: abort properly upon errors
  merge-recursive: avoid returning a wholesale struct
  merge-recursive: allow write_tree_from_memory() to error out
  merge-recursive: handle return values indicating errors
  merge-recursive: switch to returning errors instead of dying
  am -3: use merge_recursive() directly again
  merge-recursive: flush output buffer before printing error messages
  merge-recursive: write the commit title in one go
  merge-recursive: offer an option to retain the output in 'obuf'
  Ensure that the output buffer is released after calling merge_trees()
  merge-recursive: flush output buffer even when erroring out

 builtin/am.c   |  62 ++
 builtin/checkout.c |   5 +-
 builtin/ls-files.c |   3 +-
 builtin/merge.c|   2 +
 builtin/update-index.c |   2 +-
 grep.c |   8 +-
 imap-send.c|   2 +-
 merge-recursive.c  | 578 +
 merge-recursive.h  |   2 +-
 sequencer.c|   5 +
 sha1_file.c|   4 +-
 t/t5520-pull.sh|  32 +++
 trailer.c  |   2 +-
 transport.c|   2 +-
 wt-status.c|   4 +-
 15 files changed, 418 insertions(+), 295 deletions(-)

Published-As: 
https://github.com/dscho/git/releases/tag/am-3-merge-recursive-direct-v5
Interdiff vs v4:

 diff --git a/imap-send.c b/imap-send.c
 index 67d67f8..0f5f476 100644
 --- a/imap-send.c
 +++ b/imap-send.c
 @@ -506,12 +506,12 @@ static char *next_arg(char **s)
  
  static int nfsnprintf(char *buf, int blen, const char *fmt, ...)
  {
 -  int ret = -1;
 +  int ret;
va_list va;
  
va_start(va, fmt);
if (blen <= 0 || (unsigned)(ret = vsnprintf(buf, blen, fmt, va)) >= 
(unsigned)blen)
 -  die("BUG: buffer too small (%d < %d)", ret, blen);
 +  die("BUG: buffer too small. Please report a bug.");
va_end(va);
return ret;
  }
 diff --git a/merge-recursive.c b/merge-recursive.c
 index a3d12e6..66e93e0 100644
 --- a/merge-recursive.c
 +++ b/merge-recursive.c
 @@ -2041,9 +2041,10 @@ int merge_recursive(struct merge_options *o,
/*
 * When the merge fails, the result contains files
 * with conflict markers. The cleanness flag is
 -   * ignored, it was never actually used, as result of
 -   * merge_trees has always overwritten it: the committed
 -   * "conflicts" were already resolved.
 +   * ignored (unless indicating an error), it was never
 +   * actually used, as result of merge_trees has always
 +   * 

[PATCH v5 04/16] merge-recursive: clarify code in was_tracked()

[Johannes Schindelin]

It can be puzzling to see that was_tracked() asks to get an index entry
by name, but does not take a negative return value for an answer.

The reason we have to do this is that cache_name_pos() only looks for
entries in stage 0, even if nobody asked for any stage in particular.

Let's rewrite the logic a little bit, to handle the easy case early: if
cache_name_pos() returned a non-negative position, we know it is a match,
and we do not even have to compare the name again (cache_name_pos() did
that for us already). We can say right away: yes, this file was tracked.

Only if there was no exact match do we need to look harder for any
matching entry in stage 2.

Signed-off-by: Johannes Schindelin 
---
 merge-recursive.c | 30 ++
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 1b6db87..3a652b7 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -667,23 +667,21 @@ static int was_tracked(const char *path)
 {
int pos = cache_name_pos(path, strlen(path));
 
-   if (pos < 0)
-   pos = -1 - pos;
-   while (pos < active_nr &&
-  !strcmp(path, active_cache[pos]->name)) {
-   /*
-* If stage #0, it is definitely tracked.
-* If it has stage #2 then it was tracked
-* before this merge started.  All other
-* cases the path was not tracked.
-*/
-   switch (ce_stage(active_cache[pos])) {
-   case 0:
-   case 2:
+   if (0 <= pos)
+   /* we have been tracking this path */
+   return 1;
+
+   /*
+* Look for an unmerged entry for the path,
+* specifically stage #2, which would indicate
+* that "our" side before the merge started
+* had the path tracked (and resulted in a conflict).
+*/
+   for (pos = -1 - pos;
+pos < active_nr && !strcmp(path, active_cache[pos]->name);
+pos++)
+   if (ce_stage(active_cache[pos]) == 2)
return 1;
-   }
-   pos++;
-   }
return 0;
 }
 
-- 
2.9.0.281.g286a8d9


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 08/16] merge-recursive: allow write_tree_from_memory() to error out

[Johannes Schindelin]

It is possible that a tree cannot be written (think: disk full). We
will want to give the caller a chance to clean up instead of letting
the program die() in such a case.

Signed-off-by: Johannes Schindelin 
---
 merge-recursive.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 2be1e17..1f86338 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1888,8 +1888,8 @@ int merge_trees(struct merge_options *o,
else
clean = 1;
 
-   if (o->call_depth)
-   *result = write_tree_from_memory(o);
+   if (o->call_depth && !(*result = write_tree_from_memory(o)))
+   return -1;
 
return clean;
 }
-- 
2.9.0.281.g286a8d9


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 14/16] merge-recursive: offer an option to retain the output in 'obuf'

[Johannes Schindelin]

Since 66a155b (Enable output buffering in merge-recursive., 2007-01-14),
we already accumulate the output in a buffer. The idea was to avoid
interfering with the progress output that goes to stderr, which is
unbuffered, when we write to stdout, which is buffered.

We extend that buffering to allow the caller to handle the output
(possibly suppressing it). This will help us when extending the
sequencer to do rebase -i's brunt work: it does not want the picks to
print anything by default but instead determine itself whether to print
the output or not.

Note that we also redirect the error messages into the output buffer
when the caller asked not to flush the output buffer, for two reasons:
1) to retain the correct output order, and 2) to allow the caller to
suppress *all* output.

Signed-off-by: Johannes Schindelin 
---
 merge-recursive.c | 17 +
 merge-recursive.h |  2 +-
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 723b8d0..311cfa4 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -25,7 +25,7 @@
 
 static void flush_output(struct merge_options *o)
 {
-   if (o->obuf.len) {
+   if (o->buffer_output < 2 && o->obuf.len) {
fputs(o->obuf.buf, stdout);
strbuf_reset(>obuf);
}
@@ -36,10 +36,19 @@ static int err(struct merge_options *o, const char *err, 
...)
va_list params;
 
va_start(params, err);
-   flush_output(o);
+   if (o->buffer_output < 2)
+   flush_output(o);
+   else {
+   strbuf_complete(>obuf, '\n');
+   strbuf_addstr(>obuf, "error: ");
+   }
strbuf_vaddf(>obuf, err, params);
-   error("%s", o->obuf.buf);
-   strbuf_reset(>obuf);
+   if (o->buffer_output > 1)
+   strbuf_addch(>obuf, '\n');
+   else {
+   error("%s", o->obuf.buf);
+   strbuf_reset(>obuf);
+   }
va_end(params);
 
return -1;
diff --git a/merge-recursive.h b/merge-recursive.h
index d415724..340704c 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -13,7 +13,7 @@ struct merge_options {
MERGE_RECURSIVE_THEIRS
} recursive_variant;
const char *subtree_shift;
-   unsigned buffer_output : 1;
+   unsigned buffer_output : 2; /* 1: output at end, 2: keep buffered */
unsigned renormalize : 1;
long xdl_opts;
int verbosity;
-- 
2.9.0.281.g286a8d9


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 8/8] status: tests for --porcelain=v2

[Johannes Schindelin]

Hi Jeff,

On Mon, 25 Jul 2016, Jeff Hostetler wrote:

> +##
> +## Confirm VVP output prior to initial commit.
> +##

s/VVP/porcelain v2/...

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 05/16] Prepare the builtins for a libified merge_recursive()

[Johannes Schindelin]

Previously, callers of merge_trees() or merge_recursive() expected that
code to die() with an error message. This used to be okay because we
called those commands from scripts, and had a chance to print out a
message in case the command failed fatally (read: with exit code 128).

As scripting incurs its own set of problems (portability, speed,
idiosynchracies of different shells, limited data structures leading to
inefficient code), we are converting more and more of these scripts into
builtins, using library functions directly.

We already tried to use merge_recursive() directly in the builtin
git-am, for example. Unfortunately, we had to roll it back temporarily
because some of the code in merge-recursive.c still deemed it okay to
call die(), when the builtin am code really wanted to print out a useful
advice after the merge failed fatally. In the next commits, we want to
fix that.

The code touched by this commit expected merge_trees() to die() with
some useful message when there is an error condition, but merge_trees()
is going to be improved by converting all die() calls to return error()
instead (i.e. return value -1 after printing out the message as before),
so that the caller can react more flexibly.

This is a step to prepare for the version of merge_trees() that no
longer dies,  even if we just imitate the previous behavior by calling
exit(128): this is what callers of e.g. `git merge` have come to expect.

Note that the callers of the sequencer (revert and cherry-pick) already
fail fast even for the return value -1; The only difference is that they
now get a chance to say " failed".

A caller of merge_trees() might want handle error messages themselves
(or even suppress them). As this patch is already complex enough, we
leave that change for a later patch.

Signed-off-by: Johannes Schindelin 
---
 builtin/checkout.c | 4 +++-
 builtin/merge.c| 2 ++
 sequencer.c| 4 
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 27c1a05..07dea3b 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -567,8 +567,10 @@ static int merge_working_tree(const struct checkout_opts 
*opts,
o.ancestor = old->name;
o.branch1 = new->name;
o.branch2 = "local";
-   merge_trees(, new->commit->tree, work,
+   ret = merge_trees(, new->commit->tree, work,
old->commit->tree, );
+   if (ret < 0)
+   exit(128);
ret = reset_tree(new->commit->tree, opts, 0,
 writeout_error);
if (ret)
diff --git a/builtin/merge.c b/builtin/merge.c
index 19b3bc2..148a9a5 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -673,6 +673,8 @@ static int try_merge_strategy(const char *strategy, struct 
commit_list *common,
hold_locked_index(, 1);
clean = merge_recursive(, head,
remoteheads->item, reversed, );
+   if (clean < 0)
+   exit(128);
if (active_cache_changed &&
write_locked_index(_index, , COMMIT_LOCK))
die (_("unable to write %s"), get_index_file());
diff --git a/sequencer.c b/sequencer.c
index cdfac82..286a435 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -293,6 +293,8 @@ static int do_recursive_merge(struct commit *base, struct 
commit *next,
clean = merge_trees(,
head_tree,
next_tree, base_tree, );
+   if (clean < 0)
+   return clean;
 
if (active_cache_changed &&
write_locked_index(_index, _lock, COMMIT_LOCK))
@@ -559,6 +561,8 @@ static int do_pick_commit(struct commit *commit, struct 
replay_opts *opts)
if (!opts->strategy || !strcmp(opts->strategy, "recursive") || 
opts->action == REPLAY_REVERT) {
res = do_recursive_merge(base, next, base_label, next_label,
 head, , opts);
+   if (res < 0)
+   return res;
write_message(, git_path_merge_msg());
} else {
struct commit_list *common = NULL;
-- 
2.9.0.281.g286a8d9


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 09/16] merge-recursive: handle return values indicating errors

[Johannes Schindelin]

We are about to libify the recursive merge machinery, where we only
die() in case of a bug or memory contention. To that end, we must heed
negative return values as indicating errors.

This requires our functions to be careful to pass through error
conditions in call chains, and for quite a few functions this means
that they have to return values to begin with.

The next step will be to convert the places where we currently die() to
return negative values (read: -1) instead.

Note that we ignore errors reported by make_room_for_path(), consistent
with the previous behavior (update_file_flags() used the return value of
make_room_for_path() only to indicate an early return, but not a fatal
error): if the error is really a fatal error, we will notice later; If
not, it was not that serious a problem to begin with. (Witnesses in
favor of this reasoning are t4151-am-abort and t7610-mergetool, which
would start failing if we stopped on errors reported by
make_room_for_path()).

Also note: while this patch makes the code slightly less readable in
update_file_flags() (we introduce a new "goto free_buf;" instead of
an explicit "free(buf); return;"), it is a preparatory change for
the next patch where we will convert all of the die() calls in the same
function to go through the free_buf return path instead.

Signed-off-by: Johannes Schindelin 
---
 merge-recursive.c | 252 --
 1 file changed, 150 insertions(+), 102 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 1f86338..6beb1e4 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -742,12 +742,12 @@ static int make_room_for_path(struct merge_options *o, 
const char *path)
return error(msg, path, _(": perhaps a D/F conflict?"));
 }
 
-static void update_file_flags(struct merge_options *o,
- const struct object_id *oid,
- unsigned mode,
- const char *path,
- int update_cache,
- int update_wd)
+static int update_file_flags(struct merge_options *o,
+const struct object_id *oid,
+unsigned mode,
+const char *path,
+int update_cache,
+int update_wd)
 {
if (o->call_depth)
update_wd = 0;
@@ -783,8 +783,7 @@ static void update_file_flags(struct merge_options *o,
 
if (make_room_for_path(o, path) < 0) {
update_wd = 0;
-   free(buf);
-   goto update_index;
+   goto free_buf;
}
if (S_ISREG(mode) || (!has_symlinks && S_ISLNK(mode))) {
int fd;
@@ -807,20 +806,22 @@ static void update_file_flags(struct merge_options *o,
} else
die(_("do not know what to do with %06o %s '%s'"),
mode, oid_to_hex(oid), path);
+ free_buf:
free(buf);
}
  update_index:
if (update_cache)
add_cacheinfo(mode, oid, path, 0, update_wd, 
ADD_CACHE_OK_TO_ADD);
+   return 0;
 }
 
-static void update_file(struct merge_options *o,
-   int clean,
-   const struct object_id *oid,
-   unsigned mode,
-   const char *path)
+static int update_file(struct merge_options *o,
+  int clean,
+  const struct object_id *oid,
+  unsigned mode,
+  const char *path)
 {
-   update_file_flags(o, oid, mode, path, o->call_depth || clean, 
!o->call_depth);
+   return update_file_flags(o, oid, mode, path, o->call_depth || clean, 
!o->call_depth);
 }
 
 /* Low level file merging, update and removal */
@@ -1019,7 +1020,7 @@ static int merge_file_one(struct merge_options *o,
return merge_file_1(o, , , , branch1, branch2, mfi);
 }
 
-static void handle_change_delete(struct merge_options *o,
+static int handle_change_delete(struct merge_options *o,
 const char *path,
 const struct object_id *o_oid, int o_mode,
 const struct object_id *a_oid, int a_mode,
@@ -1027,6 +1028,7 @@ static void handle_change_delete(struct merge_options *o,
 const char *change, const char *change_past)
 {
char *renamed = NULL;
+   int ret = 0;
if (dir_in_way(path, !o->call_depth)) {
renamed = unique_path(o, path, a_oid ? o->branch1 : o->branch2);
}
@@ -1037,21 +1039,23 @@ static void handle_change_delete(struct merge_options 
*o,
 * correct; since there is no true "middle point" between
 * them, simply reuse the 

[PATCH v5 06/16] merge_recursive: abort properly upon errors

[Johannes Schindelin]

There are a couple of places where return values never indicated errors
before, as wie simply died instead of returning.

But now negative return values mean that there was an error and we have to
abort the operation. Let's do exactly that.

Signed-off-by: Johannes Schindelin 
---
 merge-recursive.c | 17 -
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 3a652b7..58ced25 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1949,17 +1949,19 @@ int merge_recursive(struct merge_options *o,
/*
 * When the merge fails, the result contains files
 * with conflict markers. The cleanness flag is
-* ignored, it was never actually used, as result of
-* merge_trees has always overwritten it: the committed
-* "conflicts" were already resolved.
+* ignored (unless indicating an error), it was never
+* actually used, as result of merge_trees has always
+* overwritten it: the committed "conflicts" were
+* already resolved.
 */
discard_cache();
saved_b1 = o->branch1;
saved_b2 = o->branch2;
o->branch1 = "Temporary merge branch 1";
o->branch2 = "Temporary merge branch 2";
-   merge_recursive(o, merged_common_ancestors, iter->item,
-   NULL, _common_ancestors);
+   if (merge_recursive(o, merged_common_ancestors, iter->item,
+   NULL, _common_ancestors) < 0)
+   return -1;
o->branch1 = saved_b1;
o->branch2 = saved_b2;
o->call_depth--;
@@ -1975,6 +1977,8 @@ int merge_recursive(struct merge_options *o,
o->ancestor = "merged common ancestors";
clean = merge_trees(o, h1->tree, h2->tree, 
merged_common_ancestors->tree,
);
+   if (clean < 0)
+   return clean;
 
if (o->call_depth) {
*result = make_virtual_commit(mrtree, "merged tree");
@@ -2031,6 +2035,9 @@ int merge_recursive_generic(struct merge_options *o,
hold_locked_index(lock, 1);
clean = merge_recursive(o, head_commit, next_commit, ca,
result);
+   if (clean < 0)
+   return clean;
+
if (active_cache_changed &&
write_locked_index(_index, lock, COMMIT_LOCK))
return error(_("Unable to write index."));
-- 
2.9.0.281.g286a8d9


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 07/16] merge-recursive: avoid returning a wholesale struct

[Johannes Schindelin]

It is technically allowed, as per C89, for functions' return type to
be complete structs (i.e. *not* just pointers to structs).

However, it was just an oversight of this developer when converting
Python code to C code in 6d297f8 (Status update on merge-recursive in
C, 2006-07-08) which introduced such a return type.

Besides, by converting this construct to pass in the struct, we can now
start returning a value that can indicate errors in future patches. This
will help the current effort to libify merge-recursive.c.

Signed-off-by: Johannes Schindelin 
---
 merge-recursive.c | 106 --
 1 file changed, 56 insertions(+), 50 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 58ced25..2be1e17 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -894,47 +894,47 @@ static int merge_3way(struct merge_options *o,
return merge_status;
 }
 
-static struct merge_file_info merge_file_1(struct merge_options *o,
+static int merge_file_1(struct merge_options *o,
   const struct diff_filespec *one,
   const struct diff_filespec *a,
   const struct diff_filespec *b,
   const char *branch1,
-  const char *branch2)
+  const char *branch2,
+  struct merge_file_info *result)
 {
-   struct merge_file_info result;
-   result.merge = 0;
-   result.clean = 1;
+   result->merge = 0;
+   result->clean = 1;
 
if ((S_IFMT & a->mode) != (S_IFMT & b->mode)) {
-   result.clean = 0;
+   result->clean = 0;
if (S_ISREG(a->mode)) {
-   result.mode = a->mode;
-   oidcpy(, >oid);
+   result->mode = a->mode;
+   oidcpy(>oid, >oid);
} else {
-   result.mode = b->mode;
-   oidcpy(, >oid);
+   result->mode = b->mode;
+   oidcpy(>oid, >oid);
}
} else {
if (!oid_eq(>oid, >oid) && !oid_eq(>oid, >oid))
-   result.merge = 1;
+   result->merge = 1;
 
/*
 * Merge modes
 */
if (a->mode == b->mode || a->mode == one->mode)
-   result.mode = b->mode;
+   result->mode = b->mode;
else {
-   result.mode = a->mode;
+   result->mode = a->mode;
if (b->mode != one->mode) {
-   result.clean = 0;
-   result.merge = 1;
+   result->clean = 0;
+   result->merge = 1;
}
}
 
if (oid_eq(>oid, >oid) || oid_eq(>oid, >oid))
-   oidcpy(, >oid);
+   oidcpy(>oid, >oid);
else if (oid_eq(>oid, >oid))
-   oidcpy(, >oid);
+   oidcpy(>oid, >oid);
else if (S_ISREG(a->mode)) {
mmbuffer_t result_buf;
int merge_status;
@@ -946,64 +946,66 @@ static struct merge_file_info merge_file_1(struct 
merge_options *o,
die(_("Failed to execute internal merge"));
 
if (write_sha1_file(result_buf.ptr, result_buf.size,
-   blob_type, result.oid.hash))
+   blob_type, result->oid.hash))
die(_("Unable to add %s to database"),
a->path);
 
free(result_buf.ptr);
-   result.clean = (merge_status == 0);
+   result->clean = (merge_status == 0);
} else if (S_ISGITLINK(a->mode)) {
-   result.clean = merge_submodule(result.oid.hash,
+   result->clean = merge_submodule(result->oid.hash,
   one->path,
   one->oid.hash,
   a->oid.hash,
   b->oid.hash,
   !o->call_depth);
} else if (S_ISLNK(a->mode)) {
-   oidcpy(, >oid);
+   oidcpy(>oid, >oid);
 
if (!oid_eq(>oid, >oid))
-   result.clean = 0;
+   

[PATCH v5 01/16] t5520: verify that `pull --rebase` shows the helpful advice when failing

[Johannes Schindelin]

It was noticed by Brendan Forster last October that the builtin `git am`
regressed on that. Our hot fix reverted to spawning the recursive merge
instead of using it as a library function.

As we are about to revert that hot fix, after making the recursive merge a
true library function (i.e. a function that does not die() in case of
"normal" errors), let's add a test that verifies that we do not regress on
the same problem which made the hot fix necessary in the first place.

Signed-off-by: Johannes Schindelin 
---
 t/t5520-pull.sh | 32 
 1 file changed, 32 insertions(+)

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 37ebbcf..6ad37b5 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -255,6 +255,38 @@ test_expect_success '--rebase' '
test new = "$(git show HEAD:file2)"
 '
 
+test_expect_success '--rebase with conflicts shows advice' '
+   test_when_finished "git rebase --abort; git checkout -f to-rebase" &&
+   git checkout -b seq &&
+   test_seq 5 >seq.txt &&
+   git add seq.txt &&
+   test_tick &&
+   git commit -m "Add seq.txt" &&
+   echo 6 >>seq.txt &&
+   test_tick &&
+   git commit -m "Append to seq.txt" seq.txt &&
+   git checkout -b with-conflicts HEAD^ &&
+   echo conflicting >>seq.txt &&
+   test_tick &&
+   git commit -m "Create conflict" seq.txt &&
+   test_must_fail git pull --rebase . seq 2>err >out &&
+   grep "When you have resolved this problem" out
+'
+
+test_expect_success 'failed --rebase shows advice' '
+   test_when_finished "git rebase --abort; git checkout -f to-rebase" &&
+   git checkout -b diverging &&
+   test_commit attributes .gitattributes "* text=auto" attrs &&
+   sha1="$(printf "1\\r\\n" | git hash-object -w --stdin)" &&
+   git update-index --cacheinfo 0644 $sha1 file &&
+   git commit -m v1-with-cr &&
+   # force checkout because `git reset --hard` will not leave clean `file`
+   git checkout -f -b fails-to-rebase HEAD^ &&
+   test_commit v2-without-cr file "2" file2-lf &&
+   test_must_fail git pull --rebase . diverging 2>err >out &&
+   grep "When you have resolved this problem" out
+'
+
 test_expect_success '--rebase fails with multiple branches' '
git reset --hard before-rebase &&
test_must_fail git pull --rebase . copy master 2>err &&
-- 
2.9.0.281.g286a8d9


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 03/16] Avoid translating bug messages

[Johannes Schindelin]

While working on the patch series that avoids die()ing in recursive
merges, the issue came up that bug reports (i.e. die("BUG: ...")
constructs) should never be translated, as the target audience is the
Git developer community, not necessarily the current user, and hence
a translated message would make it *harder* to address the problem.

So let's stop translating the obvious ones. As it is really, really
outside the purview of this patch series to see whether there are more
die() statements that report bugs and are currently translated, that
task is left for another day and patch.

Signed-off-by: Johannes Schindelin 
---
 merge-recursive.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 4338b73..1b6db87 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -967,7 +967,7 @@ static struct merge_file_info merge_file_1(struct 
merge_options *o,
if (!oid_eq(>oid, >oid))
result.clean = 0;
} else
-   die(_("BUG: unsupported object type in the tree"));
+   die("BUG: unsupported object type in the tree");
}
 
return result;
@@ -1811,7 +1811,7 @@ static int process_entry(struct merge_options *o,
 */
remove_file(o, 1, path, !a_mode);
} else
-   die(_("BUG: fatal merge failure, shouldn't happen."));
+   die("BUG: fatal merge failure, shouldn't happen.");
 
return clean_merge;
 }
@@ -1869,7 +1869,7 @@ int merge_trees(struct merge_options *o,
for (i = 0; i < entries->nr; i++) {
struct stage_data *e = entries->items[i].util;
if (!e->processed)
-   die(_("BUG: unprocessed path??? %s"),
+   die("BUG: unprocessed path??? %s",
entries->items[i].string);
}
 
-- 
2.9.0.281.g286a8d9


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 02/16] Report bugs consistently

[Johannes Schindelin]

The vast majority of error messages in Git's source code which report a
bug use the convention to prefix the message with "BUG:".

As part of cleaning up merge-recursive to stop die()ing except in case of
detected bugs, let's just make the remainder of the bug reports consistent
with the de facto rule.

Signed-off-by: Johannes Schindelin 
---
 builtin/ls-files.c |  3 ++-
 builtin/update-index.c |  2 +-
 grep.c |  8 
 imap-send.c|  2 +-
 merge-recursive.c  | 15 +++
 sha1_file.c|  4 ++--
 trailer.c  |  2 +-
 transport.c|  2 +-
 wt-status.c|  4 ++--
 9 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index f02e3d2..00ea91a 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -118,7 +118,8 @@ static void show_killed_files(struct dir_struct *dir)
 */
pos = cache_name_pos(ent->name, ent->len);
if (0 <= pos)
-   die("bug in show-killed-files");
+   die("BUG: killed-file %.*s not found",
+   ent->len, ent->name);
pos = -pos - 1;
while (pos < active_nr &&
   ce_stage(active_cache[pos]))
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 6cdfd5f..ba04b19 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1146,7 +1146,7 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
report(_("Untracked cache enabled for '%s'"), 
get_git_work_tree());
break;
default:
-   die("Bug: bad untracked_cache value: %d", untracked_cache);
+   die("BUG: bad untracked_cache value: %d", untracked_cache);
}
 
if (active_cache_changed) {
diff --git a/grep.c b/grep.c
index 394c856..22cbb73 100644
--- a/grep.c
+++ b/grep.c
@@ -693,10 +693,10 @@ static struct grep_expr *prep_header_patterns(struct 
grep_opt *opt)
 
for (p = opt->header_list; p; p = p->next) {
if (p->token != GREP_PATTERN_HEAD)
-   die("bug: a non-header pattern in grep header list.");
+   die("BUG: a non-header pattern in grep header list.");
if (p->field < GREP_HEADER_FIELD_MIN ||
GREP_HEADER_FIELD_MAX <= p->field)
-   die("bug: unknown header field %d", p->field);
+   die("BUG: unknown header field %d", p->field);
compile_regexp(p, opt);
}
 
@@ -709,7 +709,7 @@ static struct grep_expr *prep_header_patterns(struct 
grep_opt *opt)
 
h = compile_pattern_atom();
if (!h || pp != p->next)
-   die("bug: malformed header expr");
+   die("BUG: malformed header expr");
if (!header_group[p->field]) {
header_group[p->field] = h;
continue;
@@ -1514,7 +1514,7 @@ static int grep_source_1(struct grep_opt *opt, struct 
grep_source *gs, int colle
case GREP_BINARY_TEXT:
break;
default:
-   die("bug: unknown binary handling mode");
+   die("BUG: unknown binary handling mode");
}
}
 
diff --git a/imap-send.c b/imap-send.c
index db0fafe..0f5f476 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -511,7 +511,7 @@ static int nfsnprintf(char *buf, int blen, const char *fmt, 
...)
 
va_start(va, fmt);
if (blen <= 0 || (unsigned)(ret = vsnprintf(buf, blen, fmt, va)) >= 
(unsigned)blen)
-   die("Fatal: buffer too small. Please report a bug.");
+   die("BUG: buffer too small. Please report a bug.");
va_end(va);
return ret;
 }
diff --git a/merge-recursive.c b/merge-recursive.c
index a4a1195..4338b73 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -268,7 +268,7 @@ struct tree *write_tree_from_memory(struct merge_options *o)
fprintf(stderr, "BUG: %d %.*s\n", ce_stage(ce),
(int)ce_namelen(ce), ce->name);
}
-   die("Bug in merge-recursive.c");
+   die("BUG: unmerged index entries in merge-recursive.c");
}
 
if (!active_cache_tree)
@@ -966,9 +966,8 @@ static struct merge_file_info merge_file_1(struct 
merge_options *o,
 
if (!oid_eq(>oid, >oid))
result.clean = 0;
-   } else {
-   die(_("unsupported object type in the tree"));
-   }
+   } else
+   

Re: [PATCH v2 7/8] status: update git-status.txt for --porcelain=v2

[Johannes Schindelin]

Hi Kuba,

On Tue, 26 Jul 2016, Jakub Narębski wrote:

> W dniu 2016-07-25 o 21:25, Jeff Hostetler pisze:
> 
> > +Field   Meaning
> > +
> > +A 2 character field describing the conflict type
> > +as described in the short format.
> > +   A 4 character field describing the submodule state
> > +as described above.
> > +The 6 character octal file modes for the stage 1,
> > +stage 2, stage 3, and worktree.
> 
> Errr... the pattern has only _3_ character octal modes,   .
> A question: what happens during octopus merge?

From git-merge-octopus.sh:

# We allow only last one to have a hand-resolvable
# conflicts.  Last round failed and we still had
# a head to merge.

In other words, octopus merges do not use higher stages than 3.

Ciao,
Dscho

[PATCH v2] merge: Run commit-msg hook

[orgads]

From: Orgad Shaneh 

commit-msg is needed to either validate the commit message or edit it.

Gerrit for instance uses this hook to append its Change-Id footer. The
hook is installed on the user's machine, and it is expected to append
the footer for each commit that the user creates.

This is relevant to merge commit just like any other commit. Currently
this hook is only called for simple commits, so Gerrit user that tries
to push a merge commit has to amend it first in order to execute the
hook that appends the footer.

prepare-commit-msg hook is already called on merge, but commit-msg isn't.
It looks like unlike pre-commit not being executed, which was a conscious
decision[1], commit-msg was never considered.

[1] http://thread.gmane.org/gmane.comp.version-control.git/151297/focus=151435

Signed-off-by: Orgad Shaneh 
---
 Documentation/git-merge.txt | 6 +-
 builtin/merge.c | 7 ++-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index b758d55..59508aa 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 [verse]
 'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]
[-s ] [-X ] [-S[]]
-   [--[no-]allow-unrelated-histories]
+   [--[no-]allow-unrelated-histories] [--no-verify]
[--[no-]rerere-autoupdate] [-m ] [...]
 'git merge'  HEAD ...
 'git merge' --abort
@@ -87,6 +87,10 @@ invocations. The automated message can include the branch 
description.
Allow the rerere mechanism to update the index with the
result of auto-conflict resolution if possible.
 
+--no-verify::
+   This option bypasses the commit-msg hook.
+   See also linkgit:githooks[5].
+
 --abort::
Abort the current conflict resolution process, and
try to reconstruct the pre-merge state.
diff --git a/builtin/merge.c b/builtin/merge.c
index b555a1b..30c03c8 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -51,7 +51,7 @@ static const char * const builtin_merge_usage[] = {
 static int show_diffstat = 1, shortlog_len = -1, squash;
 static int option_commit = 1;
 static int option_edit = -1;
-static int allow_trivial = 1, have_message, verify_signatures;
+static int allow_trivial = 1, have_message, verify_signatures, no_verify;
 static int overwrite_ignore = 1;
 static struct strbuf merge_msg = STRBUF_INIT;
 static struct strategy **use_strategies;
@@ -228,6 +228,7 @@ static struct option builtin_merge_options[] = {
{ OPTION_STRING, 'S', "gpg-sign", _commit, N_("key-id"),
  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
OPT_BOOL(0, "overwrite-ignore", _ignore, N_("update ignored 
files (default)")),
+   OPT_BOOL(0, "no-verify", _verify, N_("bypass commit-msg hook")),
OPT_END()
 };
 
@@ -809,6 +810,10 @@ static void prepare_to_commit(struct commit_list 
*remoteheads)
if (launch_editor(git_path_merge_msg(), NULL, NULL))
abort_commit(remoteheads, NULL);
}
+   if (!no_verify &&
+   run_commit_hook(0 < option_edit, get_index_file(), "commit-msg",
+   git_path_merge_msg(), NULL))
+   abort_commit(remoteheads, NULL);
read_merge_msg();
strbuf_stripspace(, 0 < option_edit);
if (!msg.len)
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] merge: Run commit-msg hook

[Orgad Shaneh]

Hi again,

On Tue, Jul 26, 2016 at 5:54 PM, Johannes Schindelin
 wrote:
>
> Hi Orgad,
>
> On Tue, 26 Jul 2016, Orgad Shaneh wrote:
>
> > On Tue, Jul 26, 2016 at 4:02 PM, Johannes Schindelin
> >  wrote:
> > >
> > > On Tue, 26 Jul 2016, Orgad Shaneh wrote:
> > >
> > >> commit-msg is needed to either validate the commit message or edit it.
> > >> Gerrit for instance uses this hook to append its Change-Id footer.
> > >>
> > >> This is relevant to merge commit just like any other commit.
> > >
> > > Hmm. This is not very convincing to me, as
> > >
> > > - if you call commit-msg in `git merge` now, why not `prepare-commit-msg`?
> >
> > prepare-commit-msg is already called, a few lines above this addition.
>
> Oh. That would have made a heck of a convincing argument in the commit
> message. Pity it was not mentioned. (Yes, please read that as a strong
> suggestion to fix that in the next patch iteration.)


Done.

>
>
> FWIW I dug out the original submission:
> http://thread.gmane.org/gmane.comp.version-control.git/151297/focus=151435
>
> It seems that there was no discussion about the commit-msg. Which makes me
> wonder why nobody thought of this.
>
> Now, you could make a case that you want to run the commit-msg hook in the
> same spirit as prepare-commit-msg (and mention that apparently nobody
> thought of that when the patch was accepted into builtin/merge.c).
>
> But then I wonder what your argument would be for *not* running the
> pre-commit and the post-commit hooks in `git merge` as well?
>
> Seems like a big inconsistency to me, one that would not be helped by a
> piecemeal patch that does only half the job of resolving the inconsistency.
>
> There was actually a question why the post-commit hook was not run in `git
> merge`:
> http://thread.gmane.org/gmane.comp.version-control.git/168716/focus=168773
> (but it seems nobody cared all that much).


pre-commit seems to have been rejected, though I must admit I don't
fully understand why.
post-commit might make sense, but I wouldn't include it in the same
patch. These are
different issues.

>
> > > - a merge is a different beast from a simple commit. That is why we
> > > have two different commands for them. A hook to edit the merge message
> > > may need to know the *second* parent commit, too, for example to
> > > generate a diffstat, or to add information about changes in an "evil
> > > commit".
> >
> > That is correct for a post-merge hook. Why should *message validation*
> > differ between simple and merge commit?
>
> You yourself do not use the hook for validation. You use it to *edit* the
> message. My examples do the very same thing. Why should they wait for a
> *post-merge* hook to amend the message?


Because commit-msg doesn't know anything about the commit. It only refers
to the message. The commit might even not exist when it is called. If you
need data from the commit, post-merge is the right place.

>
> Otherwise, why wouldn't you use the post-merge hook to add the Change-Id:
> in your use case, too...
>
> > > - if Gerrit is the intended user, would it not make more sense to
> > >   introduce a new hook, e.g. `merge-msg` (and `prepare-merge-msg`), as you
> > >   have to teach Gerrit a new trick anyway?
> >
> > Why is that new? Every commit in gerrit has a Change-Id footer, which is
> > generated by commit-msg hook.
>
> So it already works for Gerrit? Why is this patch needed, then? This is
> confusing.


Gerrit is a server, the user installs a commit-msg hook provided by
Gerrit on the local machine.
This hook currently works only for simple commits and not for
(trivial) merge commits.
That's where this patch comes to the rescue.

>
> > What I currently do for merges that succeed without conflicts is
> > unconditional commit --amend --no-edit just to run the hook.
>
> So you do that manually? Or you taught Gerrit to do that? Please clarify.


Gerrit can't do anything on my machine. It's a web/ssh server. I have my own
post-merge hook that runs commit --amend

> > > - if Gerrit is the intended user, why does it not simply edit the merge
> > >   message itself? After all, it executes it, and probably crafts a merge
> > >   message mentioning that this is an automatic merge, anyway, so why not
> > >   add the Change-Id *then*?
> >
> > Most Gerrit setups require Change-Id in the commit message that the user
> > pushes. It is possible to disable this setting, and then you don't need the
> > Change-Id at all, but then you can't push a new patch set for the change
> > (unless you copy the Change-Id from gerrit to your commit message).
> > That's a real pain, I'm not aware of any public gerrit server that
> > disables this :)
>
> Forgive me, I never used Gerrit, and neither do I intend to do so.

Too bad, great system :)

> I would appreciate a self-contained commit message that explains just
> enough about Gerrit to understand what the patch tries to solve, and in a
> manner such that even 

Re: [PATCH v1 2/3] convert: modernize tests

[Remi Galan Alfonso]

Hi Lars,

Sorry, minor nit that I noticed a couple of days ago but didn't
comment on the moment and forgot until now.

Lars Schneider  wrote:
> Use `test_config` to set the config, check that files are empty with
> `test_must_be_empty`, compare files with `test_cmp`, and remove spaces
> after ">".

Considering how close it is to your patch, you might also want to
remove spaces after '<'.

There is only one occurrence in this file and it's in a line you are
already modifying.

See below:

>  test_expect_success check '
>  
> -cmp test.o test &&
> -cmp test.o test.t &&
> +test_cmp test.o test &&
> +test_cmp test.o test.t &&
>  
>  # ident should be stripped in the repository
>  git diff --raw --exit-code :test :test.i &&
> @@ -47,10 +47,10 @@ test_expect_success check '
>  embedded=$(sed -ne "$script" test.i) &&
>  test "z$id" = "z$embedded" &&
>  
> -git cat-file blob :test.t > test.r &&
> +git cat-file blob :test.t >test.r &&
>  
> -./rot13.sh < test.o > test.t &&
> -cmp test.r test.t
> +./rot13.sh < test.o >test.t &&

Here.

> +test_cmp test.r test.t
>  '

Thanks,
Rémi
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/4] worktree: add per-worktree config files

[Duy Nguyen]

On Tue, Jul 26, 2016 at 2:59 AM, Stefan Beller  wrote:
> I like the user facing design, but how am I supposed to use it internally?
>
> Say I want to read a value preferably from the worktree I'd do a
> /*
>  * maybe I don't even have to set it to 1 as
>  * the user is supposed to do that?
>  */
> repository_format_worktree_config = 1;
> git_config_get_{string,bool,int} (... as usual ...)
>
> and if I want to read the value globally I would set the variable to 0
> and read? (I would need to restore it, so I'll have a temporary variable
> to keep the original value of repository_format_worktree_config)

I would understand if you need an api to write to worktree config or
the shared one. But choosing to _read_ from a specific source sounds
wrong. The common rule should apply everywhere: read from worktree
first, if not found, read again from shared config. Why do you need
this?
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] merge: Run commit-msg hook

[Johannes Schindelin]

Hi Orgad,

On Tue, 26 Jul 2016, Orgad Shaneh wrote:

> On Tue, Jul 26, 2016 at 4:02 PM, Johannes Schindelin
>  wrote:
> >
> > On Tue, 26 Jul 2016, Orgad Shaneh wrote:
> >
> >> commit-msg is needed to either validate the commit message or edit it.
> >> Gerrit for instance uses this hook to append its Change-Id footer.
> >>
> >> This is relevant to merge commit just like any other commit.
> >
> > Hmm. This is not very convincing to me, as
> >
> > - if you call commit-msg in `git merge` now, why not `prepare-commit-msg`?
> 
> prepare-commit-msg is already called, a few lines above this addition.

Oh. That would have made a heck of a convincing argument in the commit
message. Pity it was not mentioned. (Yes, please read that as a strong
suggestion to fix that in the next patch iteration.)

FWIW I dug out the original submission:
http://thread.gmane.org/gmane.comp.version-control.git/151297/focus=151435

It seems that there was no discussion about the commit-msg. Which makes me
wonder why nobody thought of this.

Now, you could make a case that you want to run the commit-msg hook in the
same spirit as prepare-commit-msg (and mention that apparently nobody
thought of that when the patch was accepted into builtin/merge.c).

But then I wonder what your argument would be for *not* running the
pre-commit and the post-commit hooks in `git merge` as well?

Seems like a big inconsistency to me, one that would not be helped by a
piecemeal patch that does only half the job of resolving the inconsistency.

There was actually a question why the post-commit hook was not run in `git
merge`:
http://thread.gmane.org/gmane.comp.version-control.git/168716/focus=168773
(but it seems nobody cared all that much).

> > - a merge is a different beast from a simple commit. That is why we
> > have two different commands for them. A hook to edit the merge message
> > may need to know the *second* parent commit, too, for example to
> > generate a diffstat, or to add information about changes in an "evil
> > commit".
> 
> That is correct for a post-merge hook. Why should *message validation*
> differ between simple and merge commit?

You yourself do not use the hook for validation. You use it to *edit* the
message. My examples do the very same thing. Why should they wait for a
*post-merge* hook to amend the message?

Otherwise, why wouldn't you use the post-merge hook to add the Change-Id:
in your use case, too...

> > - if Gerrit is the intended user, would it not make more sense to
> >   introduce a new hook, e.g. `merge-msg` (and `prepare-merge-msg`), as you
> >   have to teach Gerrit a new trick anyway?
> 
> Why is that new? Every commit in gerrit has a Change-Id footer, which is
> generated by commit-msg hook.

So it already works for Gerrit? Why is this patch needed, then? This is
confusing.

> What I currently do for merges that succeed without conflicts is
> unconditional commit --amend --no-edit just to run the hook.

So you do that manually? Or you taught Gerrit to do that? Please clarify.

> > - if Gerrit is the intended user, why does it not simply edit the merge
> >   message itself? After all, it executes it, and probably crafts a merge
> >   message mentioning that this is an automatic merge, anyway, so why not
> >   add the Change-Id *then*?
> 
> Most Gerrit setups require Change-Id in the commit message that the user
> pushes. It is possible to disable this setting, and then you don't need the
> Change-Id at all, but then you can't push a new patch set for the change
> (unless you copy the Change-Id from gerrit to your commit message).
> That's a real pain, I'm not aware of any public gerrit server that
> disables this :)

Forgive me, I never used Gerrit, and neither do I intend to do so.

I would appreciate a self-contained commit message that explains just
enough about Gerrit to understand what the patch tries to solve, and in a
manner such that even developers who decided to ignore Gerrit understand.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [ANN] Pro Git Reedited 2nd Edition

[Jon Forrest]

On 7/26/2016 2:15 AM, Manlio Perillo wrote:


I have noted a problem when reading the PDF with Chromium: the
anchors/links do not work.


I see what you mean. I was able to replicate the problem with Acrobat
Reader on Windows 10. This seems to happen only with internal links - links
to external URLs work fine.


I don't know if this is an issue with the conversion to PDF or an
issue with Chromium.


It's not Chromium.

This is going to be tricky to fix. I'm just using the tool chain
that Scott and Ben set up. I've found other issues too that will
be hard to resolve.

Thanks for the report. I guess for now the best thing to do is
to use the HTML version.

Jon

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 3/3] convert: add filter..useProtocol option

[Jakub Narębski]

W dniu 2016-07-25 o 22:09, Lars Schneider pisze:
> On 24 Jul 2016, at 22:14, Jakub Narębski  wrote:
>> W dniu 2016-07-24 o 20:36, Lars Schneider pisze:
>>> On 23 Jul 2016, at 02:11, Jakub Narębski  wrote:
 W dniu 2016-07-22 o 17:49, larsxschnei...@gmail.com pisze:
> From: Lars Schneider 

 Nb. this line is only needed if you want author name and/or date
 different from the email sender, or if you have sender line misconfigured
 (e.g. lacking the human readable name).
>>>
>>> I use "git format-patch" to generate these emails:
>>>
>>> git format-patch --cover-letter --subject-prefix="PATCH ..." -M $BASE -o 
>>> $OUTPUT
>>>
>>> How would I disable this line? (I already checked the man page to no avail).
>>
>> If you are using `git send-email` or equivalent, I think it is
>> stripped automatically if it is not needed (in you case it was,
>> because Sender was lacking human readable name... at least I think
>> it was because of what my email reader inserted as reply line).
>> If you are using an ordinary email client, you need to remove it
>> yourself, if needed.
> 
> Weird. I am sending the patches with this command:
> 
> git send-email mystuff/* --to=git@vger.kernel.org --cc=...
> 
> Maybe I need to try the "--suppress-from" switch?!

No, the "From:" line is needed, but only because your Sender seems
to be lacking human-readable name (for some strange reason).

But let's stop this here.
 

[...]
 I also agree that we might want to be able to keep clean and smudge
 filters separate, but be able to run a single program if they are
 both the same. I think there is a special case for filter unset,
 and/or filter being "cat" -- we would want to keep that.
>>>
>>> Since 1a8630d there is a more efficient way to unset a filter ;-)
>>> Can you think of other cases where the separation would be useful?
>>
>> I can't think of any, but it doesn't mean that it does not exist.
>> It also does not mean that you need to consider situation that may
>> not happen. Covering one-way filters, like "indent" filter for `clean`,
>> should be enough... they do work with your proposal, don't they?
> 
> This should work right now but it would be a bit inefficient (the filter
> would just pass the data unchanged through the smudge command). I plan to
> add a "capabilities" flag to the protocol. Then you can define only
> the "clean" capability and nothing or the current filter mechanism 
> would happen for smudge (I will make a test case to demonstrate that
> behavior in v2).

Isn't no-op filter (value not set, value set to empty string, "cat")
caught earlier in a common code?  We would certainly want to keep
one-way filter configuration mechanism without many changes.

Also, this should be of course tested.
 
Git <-- Filter: "capabilities clean smudge\n"

 Or we can add capabilities in later version...
>>>
>>> That is an interesting idea. My initial thought was to make the capabilities
>>> of a certain version fix. If we want to add new capabilities then we would 
>>> bump the version. I wonder what others think about your suggestion!
>>
>> Using capabilities (like git-upload-pack / git-receive-pack, that is
>> smart Git transfer protocols do) is probably slightly more difficult on
>> the Git side (assuming no capabilities negotiation), but also much more
>> flexible than pure version numbers.
>>
>> One possible idea for a capability is support for passing input
>> and output of a filter via filesystem, like cleanToFile and smudgeFromFile
>> proposal in 'jh/clean-smudge-annex' (in 'pu').
>>
>> For example:
>>
>>  Git <-- Filter: "capabilities clean smudge cleanToFile smudgeFromFile\n"
> 
> Yes, I like that very much. As stated above, I will add that in v2.

I guess that you would add the idea of capabilities (though this
could be left for v3), not support for "cleanToFile" / "smudgeFromFile"
capabilities, and accompanying extension to the protocol, isn't it?
 
 
>   Git --> Filter: "testfile.dat\n"

 Unfortunately, while sane filenames should not contain newlines[1],
 the unfortunate fact is that *filenames can include newlines*, and
 you need to be able to handle that[2].  Therefore you need either to
 choose a different separator (the only one that can be safely used
 is "\0", i.e. the NUL character - but it is not something easy to
 handle by shell scripts), or C-quote filenames as needed, or always
 C-quote filenames.  C-quoting at minimum should include quoting newline
 character, and the escape character itself.

 BTW. is it the basename of a file, or a full pathname?

 [1]: http://www.dwheeler.com/essays/fixing-unix-linux-filenames.html
 [2]: http://www.dwheeler.com/essays/filenames-in-shell.html
>>>
>>> Thanks for this explanation. A bash version of the protocol is not
>>> trivial (I tried it but ended up using Perl). Therefore I think "\0"
>>> 

[PATCH v2] commit: Fix description of no-verify

[orgads]

From: Orgad Shaneh 

include also commit-msg hook.

This brings the short help in line with the documentation.

Signed-off-by: Orgad Shaneh 
---
 builtin/commit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 163dbca..2725712 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1616,7 +1616,7 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
OPT_BOOL(0, "interactive", , N_("interactively add 
files")),
OPT_BOOL('p', "patch", _interactive, N_("interactively 
add changes")),
OPT_BOOL('o', "only", , N_("commit only specified files")),
-   OPT_BOOL('n', "no-verify", _verify, N_("bypass pre-commit 
hook")),
+   OPT_BOOL('n', "no-verify", _verify, N_("bypass pre-commit 
and commit-msg hooks")),
OPT_BOOL(0, "dry-run", _run, N_("show what would be 
committed")),
OPT_SET_INT(0, "short", _format, N_("show status 
concisely"),
STATUS_FORMAT_SHORT),
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/8] status: print per-file porcelain v2 status data

[Jeff Hostetler]

On 07/25/2016 04:23 PM, Junio C Hamano wrote:

Jeff Hostetler  writes:


+static void wt_porcelain_v2_print(struct wt_status *s);
+


There is no point in this forward declaration, if you just place the
implementation of these functions here, no?


Right. I just did it that way to make the diffs with the previous
commit draw a little cleaner.  But I can take it out.



+/*
+ * Print porcelain v2 info for tracked entries with changes.
+ */
+static void wt_porcelain_v2_print_changed_entry(
+   struct string_list_item *it,
+   struct wt_status *s)
+{
+...
+   fprintf(s->fp, "%c %s %s %06o %06o %06o %s %s R%d %s",


It is misleading to always say R in the output when there is no
rename, isn't it?


Yes, especially if we add it for copied entries too.
I was just looking for a way to have a fixed format.
If we make the R%d field optional, then the first pathname
is ambiguous.  That gets me back to an earlier draft where
we have rename and non-rename line types.

I'll split this up into 1 pathname and 2 pathname forms,
and only include the R%d (or the C%d) field in the latter.




+* Note that this is a last-one-wins for each the individual
+* stage [123] columns in the event of multiple cache rows
+* for a stage.


Just FYI, the usual lingo we use for that is "multiple cache entries
for the same stage", I would think.


thanks.




+*/
+   memset(stages, 0, sizeof(stages));
+   sum = 0;
+   pos = cache_name_pos(it->string, strlen(it->string));
+   assert(pos < 0);
+   pos = -pos-1;
+   while (pos < active_nr) {
+   ce = active_cache[pos++];
+   stage = ce_stage(ce);
+   if (strcmp(ce->name, it->string) || !stage)
+   break;
+   stages[stage - 1].mode = ce->ce_mode;
+   hashcpy(stages[stage - 1].oid.hash, ce->sha1);
+   sum++;
+   }
+   if (!sum)
+   die("BUG: unmerged entry without any stages");


Hmm, we seem to already have d->stagemask; if you call that variable
"sum" anyway, perhaps its computation can be more like

sum |= 1 << (stage - 1);

so that you can compare it with d->stagemask for this sanity check?


good point.

thanks
jeff


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] merge: Run commit-msg hook

[Orgad Shaneh]

Hi,

On Tue, Jul 26, 2016 at 4:02 PM, Johannes Schindelin
 wrote:
> Hi Orgad,
>
> On Tue, 26 Jul 2016, Orgad Shaneh wrote:
>
>> From: Orgad Shaneh 
>
> Again, this is unnecessary if you already send the mail from the same
> address.
>
>> commit-msg is needed to either validate the commit message or edit it.
>> Gerrit for instance uses this hook to append its Change-Id footer.
>>
>> This is relevant to merge commit just like any other commit.
>
> Hmm. This is not very convincing to me, as
>
> - if you call commit-msg in `git merge` now, why not `prepare-commit-msg`?

prepare-commit-msg is already called, a few lines above this addition.

>
> - a merge is a different beast from a simple commit. That is why we have
>   two different commands for them. A hook to edit the merge message may
>   need to know the *second* parent commit, too, for example to generate
>   a diffstat, or to add information about changes in an "evil commit".

That is correct for a post-merge hook. Why should *message validation* differ
between simple and merge commit?

> - if Gerrit is the intended user, would it not make more sense to
>   introduce a new hook, e.g. `merge-msg` (and `prepare-merge-msg`), as you
>   have to teach Gerrit a new trick anyway?

Why is that new? Every commit in gerrit has a Change-Id footer, which is
generated by commit-msg hook. What I currently do for merges that succeed
without conflicts is unconditional commit --amend --no-edit just to
run the hook.
This doesn't make sense.

> - if Gerrit is the intended user, why does it not simply edit the merge
>   message itself? After all, it executes it, and probably crafts a merge
>   message mentioning that this is an automatic merge, anyway, so why not
>   add the Change-Id *then*?

Most Gerrit setups require Change-Id in the commit message that the user
pushes. It is possible to disable this setting, and then you don't need the
Change-Id at all, but then you can't push a new patch set for the change
(unless you copy the Change-Id from gerrit to your commit message).
That's a real pain, I'm not aware of any public gerrit server that
disables this :)

> Ciao,
> Dscho

- Orgad
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] commit: Fix description of no-verify

[Johannes Schindelin]

Hi Orgad,

On Tue, 26 Jul 2016, Orgad Shaneh wrote:

> On Tue, Jul 26, 2016 at 3:55 PM, Johannes Schindelin
>  wrote:
> >
> > On Tue, 26 Jul 2016, Orgad Shaneh wrote:
> >
> >> include also commit-msg hook.
> >
> > This comment was a bit cryptic, until I read the patch. Now I find that
> > comment redundant with the patch.
> 
> This brings the short help in line with the documentation. I should
> have stated that in the commit message.

That would be good.

> I don't have much experience with submitting patches to Git. How do I
> edit the commit message? Submit it as a new patch?

You edit it locally via `git commit --amend` (or using rebase -i's
`reword` if you need to adjust a commit message in the middle of a patch
series, not at the end).

Then you send it as a reply to the first submission (--in-reply-to, if you
use format-patch), with the `[PATCH v2]` prefix (--subject-prefix='[PATCH
v2]' if you use format-patch).

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: LARGE_PACKET_MAX wrong?

[Jeff King]

On Tue, Jul 26, 2016 at 03:07:41PM +0200, Lars Schneider wrote:

> I am reading the pkt-line code and stumbled across this oddity:
> 
> LARGE_PACKET_MAX is defined as 65520
> https://github.com/git/git/blob/8c6d1f9807c67532e7fb545a944b064faff0f70b/pkt-line.h#L79
> 
> In `format_packet` we check that the 4 bytes of length data plus payload is 
> not larger than LARGE_PACKET_MAX (= 65520)
> https://github.com/git/git/blob/8c6d1f9807c67532e7fb545a944b064faff0f70b/pkt-line.c#L111-L112
> 
> However, in the documentation we state that 4 bytes of length data plus 
> payload must not exceed 65524
> https://github.com/git/git/blob/8c6d1f9807c67532e7fb545a944b064faff0f70b/Documentation/technical/protocol-common.txt#L70-L72
> 
> Who is right? Code or documentation? 

I think the documentation is wrong. Git's packet_read() will complain on
a 65524-byte incoming packet (it actually handles up to 65523, but that
is simply a quirk of the implementation).

The sending sides always include the 4-byte header in the
LARGE_PACKET_MAX calculations.

So I don't know what was intended once upon a time, but I think we have
to stick to what the code does, because there are many deployed
instances that we cannot break compatibility with.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/8] status: per-file data collection for --porcelain=v2

[Jeff Hostetler]

On 07/25/2016 04:14 PM, Junio C Hamano wrote:

Jeff Hostetler  writes:


+static void aux_updated_entry_porcelain_v2(
+   struct wt_status *s,
+   struct wt_status_change_data *d,
+   struct diff_filepair *p)
+{
+   switch (p->status) {
+   case DIFF_STATUS_ADDED:
+   /* {mode,sha1}_head are zero for an add. */
+   d->aux.porcelain_v2.mode_index = p->two->mode;


I doubt that it makes sense in the longer term to have a new "aux"
field.  Why isn't it part of the wt_status_change_data structure?
For that matter, why should these new functions have both "aux" and
"v2" in their names?

Imagine what should happen when somebody wants to add --porcelain=v3
format in 6 months.  Why must "v3" be treated differently from "v1"
and in a way close to "v2"?  Why shouldn't all the three be treated
in a similar way that "v1" has already?


I wasn't sure if we wanted the v2 fields to be isolated
and only filled in for v2 requests or whether we wanted
them to be common going forward.  In the case of the former,
I could see the "aux" struct becoming a union and the various
aux_*() routines only populating one member in that union.
And then the various per-format print routines would know
which aux member to access.  That may be more complicated
that necessary though -- if we assume that any subsequent
formats (and possibly any JSON formats) would always want
to keep these fields and add more.

I'll flatten the fields into the main structure.




+   oidcpy(>aux.porcelain_v2.oid_index, >two->oid);
+   break;
+
+   case DIFF_STATUS_DELETED:
+   d->aux.porcelain_v2.mode_head = p->one->mode;
+   oidcpy(>aux.porcelain_v2.oid_head, >one->oid);
+   /* {mode,oid}_index are zero for a delete. */
+   break;
+
+   case DIFF_STATUS_RENAMED:
+   d->aux.porcelain_v2.rename_score = p->score * 100 / MAX_SCORE;


I have a slight aversion against losing the precision in a helper
function like this that does not do the actual output, but it
probably is OK.

Don't we have copy detection score that is computed exactly the same
way for DIFF_STATUS_COPIED case, too?


Yes I believe so.  I'll see about adding that.  Or rather make
the field apply to both.



For readability, unless a case arm is completely empty, we should
have
/* fallthru */

comment where "break;" would go for a normal case arm.


will do. thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] commit: Fix description of no-verify

[Orgad Shaneh]

Hi and thanks for your reply.

On Tue, Jul 26, 2016 at 3:55 PM, Johannes Schindelin
 wrote:
> Hi Orgad
>
> On Tue, 26 Jul 2016, Orgad Shaneh wrote:
>
>> From: Orgad Shaneh 
>
> This is unnecessary, as it matches your email address.
>
>> include also commit-msg hook.
>
> This comment was a bit cryptic, until I read the patch. Now I find that
> comment redundant with the patch.

This brings the short help in line with the documentation. I should
have stated that in the commit message.

>
> However, I think that...
>
>> - OPT_BOOL('n', "no-verify", _verify, N_("bypass pre-commit 
>> hook")),
>> + OPT_BOOL('n', "no-verify", _verify, N_("bypass pre-commit 
>> and commit-msg hooks")),
>
>
> ... it may be more desirable to future-proof this simply by saying "bypass
> hooks".

That wouldn't be correct. prepare-commit-msg is not suppressed by this flag.

> In the alternative, it would be good if the commit message could
> convincingly make the case that there are no other hooks that will be
> skipped with -n.
>
> Of course, I could go and look at the source code to convince myself. But
> it is really the duty of the commit message to be already convincing
> enough.
>
> Ciao,
> Dscho

I don't have much experience with submitting patches to Git. How do I
edit the commit message? Submit it as a new patch?

- Orgad
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] merge: Run commit-msg hook

[Johannes Schindelin]

Hi Orgad,

On Tue, 26 Jul 2016, Orgad Shaneh wrote:

> From: Orgad Shaneh 

Again, this is unnecessary if you already send the mail from the same
address.

> commit-msg is needed to either validate the commit message or edit it.
> Gerrit for instance uses this hook to append its Change-Id footer.
> 
> This is relevant to merge commit just like any other commit.

Hmm. This is not very convincing to me, as

- if you call commit-msg in `git merge` now, why not `prepare-commit-msg`?

- a merge is a different beast from a simple commit. That is why we have
  two different commands for them. A hook to edit the merge message may
  need to know the *second* parent commit, too, for example to generate
  a diffstat, or to add information about changes in an "evil commit".

- if Gerrit is the intended user, would it not make more sense to
  introduce a new hook, e.g. `merge-msg` (and `prepare-merge-msg`), as you
  have to teach Gerrit a new trick anyway?

- if Gerrit is the intended user, why does it not simply edit the merge
  message itself? After all, it executes it, and probably crafts a merge
  message mentioning that this is an automatic merge, anyway, so why not
  add the Change-Id *then*?

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] t5510: skip tests under GETTEXT_POISON build

[Vasco Almeida]

Skip tests when running under GETTEXT_POISON build and run them with
C_LOCALE_OUTPUT prerequisite.

These tests are irrelevant under GETTEXT_POISON because they test text
output alignment which GETTEXT_POISON turns useless.

Signed-off-by: Vasco Almeida 
---
 t/t5510-fetch.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 6bd4853..668c54b 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -688,7 +688,7 @@ test_expect_success 'fetching with auto-gc does not lock 
up' '
)
 '
 
-test_expect_success 'fetch aligned output' '
+test_expect_success C_LOCALE_OUTPUT 'fetch aligned output' '
git clone . full-output &&
test_commit long-tag &&
(
@@ -703,7 +703,7 @@ test_expect_success 'fetch aligned output' '
test_cmp expect actual
 '
 
-test_expect_success 'fetch compact output' '
+test_expect_success C_LOCALE_OUTPUT 'fetch compact output' '
git clone . compact &&
test_commit extraaa &&
(
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] commit: Fix description of no-verify

[Johannes Schindelin]

Hi Orgad

On Tue, 26 Jul 2016, Orgad Shaneh wrote:

> From: Orgad Shaneh 

This is unnecessary, as it matches your email address.

> include also commit-msg hook.

This comment was a bit cryptic, until I read the patch. Now I find that
comment redundant with the patch.

However, I think that...

> - OPT_BOOL('n', "no-verify", _verify, N_("bypass pre-commit 
> hook")),
> + OPT_BOOL('n', "no-verify", _verify, N_("bypass pre-commit 
> and commit-msg hooks")),


... it may be more desirable to future-proof this simply by saying "bypass
hooks".

In the alternative, it would be good if the commit message could
convincingly make the case that there are no other hooks that will be
skipped with -n.

Of course, I could go and look at the source code to convince myself. But
it is really the duty of the commit message to be already convincing
enough.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 11/16] am -3: use merge_recursive() directly again

[Johannes Schindelin]

Hi Junio,

On Mon, 25 Jul 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > Note: the code now calls merge_recursive_generic() again. Unlike
> > merge_trees() and merge_recursive(), this function returns 0 upon success,
> > as most of Git's functions. Therefore, the error value -1 naturally is
> > handled correctly, and we do not have to take care of it specifically.
> 
> I've finished reading through up to this point and I'd stop for
> now.

If you want, I can break out the subsequent patches into a separate
series. I just thought that you might want to have them here, as I
implemented them in response to the concern you raised in a previous
iteration of the same patch series: you pointed out that returning a
negative error value still does not let the caller handle the error
message, and with the subsequent patches that is now possible, too.

> Some of the patches I didn't look beyond the context presented in
> the patches, so it is very possible that I missed leaks caused by
> early returns and things like that, but I didn't see anything
> glaringly wrong.  Looks very promising.

Thanks.

I did try my best to catch all resource leaks, but I did stare at those
patches so often and so long that it is easy for some obvious bug to have
slipped by. Therefore, I would appreciate it if you (or somebody as
diligent as you) could have a careful look in particular at the
"merge-recursive: switch to returning errors instead of dying" patch.

I may have missed something as stupid as an unclosed file handle, after
all.

Thank you,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html