PENDING FUNDS RELEASED.SEE ATTACHMENT

[Pending Funds]

PENDING FUNDS RELEASED.rtf
Description: Binary data

Re: Un-paged commit messages in git filter-branch's commit-filter?

[Eric Wong]

Jeff King  wrote:
> I don't recall offhand whether git-svn does line-wrapping or any other
> commit-message munging.

Definitely no line-wrapping.  Munging is minimal:
it respects i18n.commitencoding, adds a trailing newline,
and "git-svn-id:" line.
--
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/1 v2] pager: move pager-specific setup into the build

[Junio C Hamano]

Eric Wong  writes:

> From: Junio C Hamano 
>
> Allowing PAGER_ENV to be set at build-time allows us to move
> pager-specific knowledge out of our build.  Currently, this
> allows us to set a better default for FreeBSD where more(1)
> is the same binary as less(1).

Thanks for resurrecting, but I am not sure what "a better default"
is from the above description and with the patch.  Even though a
naive reading of the above (i.e. "less" and "more" are the same)
makes me expect that the patch will give the same set of default
environment settings to those on FreeBSD, you give LESS=FRX and
MORE=-R, i.e. they are configured differently.

> This also prepares us for introducing a run-time config knob to
> override the build-time environment in the next commit.

This is now gone, judging from 1/1 on the subject line being not
1/2, right?

>
> Originally-from:
>  https://public-inbox.org/git/xmqq61piw4yf@gitster.dls.corp.google.com/
>
> Signed-off-by: Eric Wong 
> ---
>  Makefile | 20 +++-
>  config.mak.uname |  1 +
>  git-sh-setup.sh  |  8 +---
>  pager.c  | 29 +
>  4 files changed, 50 insertions(+), 8 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 6a13386..0b36b5e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -370,6 +370,14 @@ all::
>  # Define HAVE_BSD_SYSCTL if your platform has a BSD-compatible sysctl 
> function.
>  #
>  # Define HAVE_GETDELIM if your system has the getdelim() function.
> +#
> +# Define PAGER_ENV to a SP separated VAR=VAL pairs to define
> +# default environment variables to be passed when a pager is spawned, e.g.
> +#
> +#PAGER_ENV = LESS=FRX LV=-c
> +#
> +# to say "export LESS=FRX (and LV=-c) if the environment variable
> +# LESS (and LV) is not set, respectively".
>  
>  GIT-VERSION-FILE: FORCE
>   @$(SHELL_PATH) ./GIT-VERSION-GEN
> @@ -1500,6 +1508,10 @@ ifeq ($(PYTHON_PATH),)
>  NO_PYTHON = NoThanks
>  endif
>  
> +ifndef PAGER_ENV
> +PAGER_ENV = LESS=FRX LV=-c
> +endif
> +
>  QUIET_SUBDIR0  = +$(MAKE) -C # space to separate -C and subdir
>  QUIET_SUBDIR1  =
>  
> @@ -1629,6 +1641,10 @@ ifdef DEFAULT_HELP_FORMAT
>  BASIC_CFLAGS += -DDEFAULT_HELP_FORMAT='"$(DEFAULT_HELP_FORMAT)"'
>  endif
>  
> +PAGER_ENV_CQ = "$(subst ",\",$(subst \,\\,$(PAGER_ENV)))"
> +PAGER_ENV_CQ_SQ = $(subst ','\'',$(PAGER_ENV_CQ))
> +BASIC_CFLAGS += -DPAGER_ENV='$(PAGER_ENV_CQ_SQ)'
> +
>  ALL_CFLAGS += $(BASIC_CFLAGS)
>  ALL_LDFLAGS += $(BASIC_LDFLAGS)
>  
> @@ -1753,7 +1769,7 @@ common-cmds.h: $(wildcard Documentation/git-*.txt)
>  
>  SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
>   $(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\
> - $(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP)
> + $(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV)
>  define cmd_munge_script
>  $(RM) $@ $@+ && \
>  sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
> @@ -1766,6 +1782,7 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
>  -e 's|@@GITWEBDIR@@|$(gitwebdir_SQ)|g' \
>  -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \
>  -e 's|@@SANE_TEXT_GREP@@|$(SANE_TEXT_GREP)|g' \
> +-e 's|@@PAGER_ENV@@|$(PAGER_ENV_SQ)|g' \
>  $@.sh >$@+
>  endef
>  
> @@ -2173,6 +2190,7 @@ GIT-BUILD-OPTIONS: FORCE
>   @echo NO_PERL=\''$(subst ','\'',$(subst ','\'',$(NO_PERL)))'\' >>$@+
>   @echo NO_PYTHON=\''$(subst ','\'',$(subst ','\'',$(NO_PYTHON)))'\' >>$@+
>   @echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst 
> ','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
> + @echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
>  ifdef TEST_OUTPUT_DIRECTORY
>   @echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst 
> ','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+
>  endif
> diff --git a/config.mak.uname b/config.mak.uname
> index 17fed2f..2484dfb 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -209,6 +209,7 @@ ifeq ($(uname_S),FreeBSD)
>   HAVE_PATHS_H = YesPlease
>   GMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes
>   HAVE_BSD_SYSCTL = YesPlease
> + PAGER_ENV = LESS=FRX LV=-c MORE=-R
>  endif
>  ifeq ($(uname_S),OpenBSD)
>   NO_STRCASESTR = YesPlease
> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index 0c34aa6..0f5a56f 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -163,9 +163,11 @@ git_pager() {
>   else
>   GIT_PAGER=cat
>   fi
> - : "${LESS=-FRX}"
> - : "${LV=-c}"
> - export LESS LV
> + for vardef in @@PAGER_ENV@@
> + do
> + var=${vardef%%=*}
> + eval ": \${$vardef} && export $var"
> + done
>  
>   eval "$GIT_PAGER" '"$@"'
>  }
> diff --git a/pager.c b/pager.c
> index 4bc0481..cd1ac54 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -63,14 +63,35 @@ const char *git_pager(int stdout_is_tty)
>   return pager;
>  }
>  
> +static void setup_pager_env(struct argv_array *env)
> +{
> + const char *pager_env = PAGER_ENV;
> +
> + 

Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build

[Eric Wong]

Junio C Hamano  wrote:
> Eric Wong  writes:
> > Allowing PAGER_ENV to be set at build-time allows us to move
> > pager-specific knowledge out of our build.  Currently, this
> > allows us to set a better default for FreeBSD where more(1)
> > is the same binary as less(1).
> 
> Thanks for resurrecting, but I am not sure what "a better default"
> is from the above description and with the patch.  Even though a
> naive reading of the above (i.e. "less" and "more" are the same)
> makes me expect that the patch will give the same set of default
> environment settings to those on FreeBSD, you give LESS=FRX and
> MORE=-R, i.e. they are configured differently.

Perhaps s/better/platform-appropriate/ ?

I just copied your original patch in setting MORE=-R
(but removed 'S' from LESS).

So v3 will be MORE=FRX, as less was added:

commit 98170c0c3ba86eb1cc975e7848d075bf2abc1ed0
Author: ps 
Date:   Mon May 22 10:00:00 2000 +

bmake glue for less.

and more was nuked:

commit cde9059fa3e4dc7e259c3864d7536252a5c580a0
Author: ps 
Date:   Mon May 29 13:31:51 2000 +

Nuke more from the repository.

And "git branch -r --contains" on both of those commits says
they showed up in the 5.0 release.  However, further
investigation says more was even gone by the 4.1.0 release

  git show origin/release/4.1.0:usr.bin/more # non-existent tree
  git show origin/release/4.0.0:usr.bin/more # tree still exists

But, "git show origin/release/4.0.0:usr.bin/more/option.c"
reveals more from those days wouldn't handle -R anyways,
and hopefully nobody is still running 4.0.0...

ref: git://github.com/freebsd/freebsd.git

> > This also prepares us for introducing a run-time config knob to
> > override the build-time environment in the next commit.
> 
> This is now gone, judging from 1/1 on the subject line being not
> 1/2, right?

Oops, yes :x

> > Originally-from:
> >  https://public-inbox.org/git/xmqq61piw4yf@gitster.dls.corp.google.com/
--
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/1 v2] pager: move pager-specific setup into the build

[Jeff King]

On Mon, Aug 01, 2016 at 04:03:40PM -0700, Junio C Hamano wrote:

> Eric Wong  writes:
> 
> > From: Junio C Hamano 
> >
> > Allowing PAGER_ENV to be set at build-time allows us to move
> > pager-specific knowledge out of our build.  Currently, this
> > allows us to set a better default for FreeBSD where more(1)
> > is the same binary as less(1).
> 
> Thanks for resurrecting, but I am not sure what "a better default"
> is from the above description and with the patch.  Even though a
> naive reading of the above (i.e. "less" and "more" are the same)
> makes me expect that the patch will give the same set of default
> environment settings to those on FreeBSD, you give LESS=FRX and
> MORE=-R, i.e. they are configured differently.

I wondered about this, too. They are the same binary, but calling less
as "more" (or setting LESS_IS_MORE) causes less to behave "like more".
Looking at the manpage, none the usual FRX options is affected. So in
theory we could just set MORE=FRX on FreeBSD.

That would be bad a idea in general, as other non-less implementations
of more might get confused.  "more" is in POSIX, and so is $MORE (and it
does not understand any of our options).

You could also make the knob "MORE_IS_LESS" or something, and just
blindly copy $LESS to $MORE. That's a bad idea, though, if somebody does
stick one of the incompatible flags in the build options.

-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: Un-paged commit messages in git filter-branch's commit-filter?

[Jeff King]

On Mon, Aug 01, 2016 at 11:49:09PM +0200, Stefan Tauner wrote:

> > For instance, if I do:
> > 
> >   git init
> >   echo content >file
> >   git add file
> >   git commit -m "$(perl -e 'print join(" ", 1..100)')"
> > 
> > I get a commit message with one long unwrapped line, which I can view
> > via git-log, etc.
> 
> That's approximately what I did in my tests as well. And like you, when
> I do this in a fresh repository, it works like that..

One thing to look at, I guess, is whether they are corrupted coming in
to the repository, or when they are being formatted.

If you do:

  git cat-file commit HEAD

you will get the raw bytes of the commit object stored by git. In the
example above, it obviously shows one long line. Have you checked that
it does so in your cases that misbehave?

> > (I wondered at first if the extra "cat" and "-m" could be messing up
> > whitespace for you, but it should not, as the quoting around "$input"
> > should preserve things like newlines. And anyway, the bug in that case
> > would be the _opposite_; I'd expect it to stuff everything onto a single
> > line rather than breaking lines).
> 
> The commit messages I try to process are nothing special really... just
> very long and not subject-like (because SVN and not giving too much
> thought to them sometimes). The only special thing I can think of is
> that they have been processed by git-svn earlier.

Hmm. The usual problem with svn-imported commits is not long lines,
exactly, but rather that the commit message has one big paragraph at the
top, rather than a subject/body split.

So when you ask git for the "subject" in such a case, it may paste many
lines together as a single one. For example:

  $ commit=$(seq 1 5 | git commit-tree HEAD^{tree})
  $ git cat-file commit $commit
  tree 07753f428765ac1afe2020b24e40785869bd4a85
  author Jeff King  1470093739 -0400
  committer Jeff King  1470093739 -0400

  1
  2
  3
  4
  5

  $ git log --format=%s $commit
  1 2 3 4 5

So could it be that your lines actually _are_ broken in the git objects,
but "%s" and other tools try to salvage them as a single subject?

I don't recall offhand whether git-svn does line-wrapping or any other
commit-message munging.

-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 bisect for reachable commits only

[Junio C Hamano]

Christian Couder  writes:

>> I think the "previous issue" was that we can ask the user to ask to
>> check one of 'x' or 'y' when given Good and Bad points in a graph like
>> this:
>>
>> x---y---y---o---B
>>  \ /
>>   x---G---o
>>
>> while a more natural expectation by a user would be that we only
>> need to check one of these two 'o'.
>
> Yeah, I reproduced the steps described in the Google Groups discussion:
>
> https://groups.google.com/forum/#!topic/git-users/v3__t42qbKE
>
> and I think that is indeed the "previous issue".
>
>> Thinking about it again, I actually think it makes sense to ask the
>> user to check 'y'; there is no good reason to believe that 'y' can
>> never have introduced the badness we are hunting for, and limiting
>> the search to --ancestry-path (which is to ask only for 'o') would
>> stop at the merge 'o' if one of the 'y' were bad, which would
>> prevents us from knowing the exact breakage.
>
> I agree.

Having agreed on that, there are cases where you do want to stop at
the merge 'o' on the upper history, when lower-history leading to B
is the mainline and you are interested in finding the merge with
which side branch introduced a breakage, and not particularly
interested in finding the exact commit on the side branch.  Upon
finding the merge 'o' that is the parent of 'B' is bad, you find out
the owner of the side branch merged there who wrote the two 'y's,
and have him work on figuring out where in his branch he broke it.

For that, the --ancestry-path is a wrong way to traverse; what we
want in that context is the --first-parent traversal.

>> There however is no excuse if we asked to check 'x', though.  They
>> are ancestors of a Good commit, and "git bisect" should be able to
>> assume they are Good.
>
> I think it does. When I reproduced the steps in the "previous issue",
> it did assume they are good.

I actually had an impression that the original report claimed that
the user was asked to check immediate parent of G, and that would be
a bug.
--
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 v9 00/41] libify apply and use lib in am, part 2

[Junio C Hamano]

Christian Couder  writes:

> On Sat, Jul 30, 2016 at 7:24 PM, Christian Couder
>  wrote:
>>
>> I will send a diff between this version and the previous one, as a
>> reply to this email.
>
> Here is the diff:

The "verbosity" bits, and also deduplication of parse_options, are
both welcome changes.

Will replace.

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] 1/4] patch-ids: stop using a hand-rolled hashmap implementation

[Eric Wong]

Junio C Hamano  wrote:
> Johannes Schindelin  writes:
> 
> > It would be a serious bug if hashmap_entry_init() played games with
> > references, given its signature (that this function does not have any
> > access to the hashmap structure, only to the entry itself):
> >
> > void hashmap_entry_init(void *entry, unsigned int hash)



> I have a slight preference to avoid the lazy "void *", but that is
> an unrelated tangent.

Me too.  I noticed this while working on the http-walker
speedups and my self-rejected last patch:

  https://public-inbox.org/git/20160711210243.GA1604%40whir/

Extracting list_entry from list.h (currently in next[1]) and
exposing that generically as "container_of" for use with
hashmap_* would make it safer-to-use and allow structs to belong
to multiple hashmaps.

list_entry is also an alias for container_of in the Linux
kernel, but we don't have enough code using list_* to
warrant two names for the same macro.

[1] https://public-inbox.org/git/20160711205131.1291-4-e%4080x24.org/
--
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] pass constants as first argument to st_mult()

[Junio C Hamano]

Jeff King  writes:

>> *1* I have a slight suspicion that this is cultural, i.e. how
>> arithmetic is taught in grade schools.  When an apple costs 30 yen
>> and I have 5 of them, I was taught to multiply 30x5 to arrive at
>> 150, not 5x30=150, and I am guessing that is because the former
>> matches the natural order of these two numbers (cost, quantity) in
>> the language I was taught.
>
> You might be right. I was trying to figure out what is "natural" for me
> in these cases, but after thinking about it for 2 minutes, I'm pretty
> sure anything resembling "natural" is lost as I try to out-think myself. :)

Do native English speakers (or more in general Europeans) think of
the apple example more like "5 apples, 30 cents each", and do 5x30?
--
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 0/1 v2] add PAGER_ENV to build

[Jeff King]

On Mon, Aug 01, 2016 at 09:49:36PM +, Eric Wong wrote:

> Changes from v1:
> 
> * dropped stringify macro in favor for quoting in Makefile
>   (diff below)
>   I'm not sure I like this change, and might be inclined to
>   go in the opposite direction of using the stringify macro
>   more widely to simplify the Makefile; but that is a separate
>   topic.

I think that's a dangerous direction. Try this:

-- >8 --
cat >foo.c <<\EOF
#include 

#define stringify_(x) #x
#define stringify(x) stringify_(x)

int main(void)
{
printf("%s", stringify(FOO));
return 0;
}
EOF

while read -r input; do
gcc -Wall -Werror -DFOO="$input" foo.c
./a.out
done
-- 8< --

and then try input like:

  this has  a lot of spaces
  this has a \backslash

You should see:

  this has a lot of spaces
  this has aackslash

I'll grant that backslashes and runs of whitespace are not things we'd
expect to find in most of our build-time config, but it still seems like
a bad direction to go (and actually, I wouldn't be surprised if
backslashes do end up in some of our build-time variables on Windows).

-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 v9 35/41] apply: make it possible to silently apply

[Junio C Hamano]

Christian Couder  writes:

> +enum apply_verbosity {
> + verbosity_silent = -1,
> + verbosity_normal = 0,
> + verbosity_verbose = 1,

Drop the trailing comma from the last element in enum definition
(comma after the last element in an array is OK).
--
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] reset cached ident date before creating objects

[Jeff King]

On Mon, Aug 01, 2016 at 02:54:47PM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > I also didn't go looking for other spots which might want similar
> > treatment. Junio mentioned that the sequencer code still uses an
> > external commit process, so cherry-pick/revert are OK. git-fast-import
> > creates a stream of commits, but already deals with this issue in a
> > separate way. And I couldn't think of other programs that want to make a
> > string of commits.
> 
> Thanks.  I wonder if do_commit() may be a more appropriate place to
> reset this, but the difference only would matter in "am --resolved",
> and a call to do_commit() it has will always be the first commit in
> the process, so there would not be anything to clear, which would in
> turn mean that it would not make any difference.

Yeah, you may be right. I didn't actually find that spot. I was focused
on the looping aspect. And seeing how complex the loop was, my thought
was to just put it at the top, so we know it happens once per iteration.
But I think do_commit() is a reasonble spot, too (as long as it comes
before the call to fmt_ident(), of course).

-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 1/1 v2] pager: move pager-specific setup into the build

[Eric Wong]

From: Junio C Hamano 

Allowing PAGER_ENV to be set at build-time allows us to move
pager-specific knowledge out of our build.  Currently, this
allows us to set a better default for FreeBSD where more(1)
is the same binary as less(1).

This also prepares us for introducing a run-time config knob to
override the build-time environment in the next commit.

Originally-from:
 https://public-inbox.org/git/xmqq61piw4yf@gitster.dls.corp.google.com/

Signed-off-by: Eric Wong 
---
 Makefile | 20 +++-
 config.mak.uname |  1 +
 git-sh-setup.sh  |  8 +---
 pager.c  | 29 +
 4 files changed, 50 insertions(+), 8 deletions(-)

diff --git a/Makefile b/Makefile
index 6a13386..0b36b5e 100644
--- a/Makefile
+++ b/Makefile
@@ -370,6 +370,14 @@ all::
 # Define HAVE_BSD_SYSCTL if your platform has a BSD-compatible sysctl function.
 #
 # Define HAVE_GETDELIM if your system has the getdelim() function.
+#
+# Define PAGER_ENV to a SP separated VAR=VAL pairs to define
+# default environment variables to be passed when a pager is spawned, e.g.
+#
+#PAGER_ENV = LESS=FRX LV=-c
+#
+# to say "export LESS=FRX (and LV=-c) if the environment variable
+# LESS (and LV) is not set, respectively".
 
 GIT-VERSION-FILE: FORCE
@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -1500,6 +1508,10 @@ ifeq ($(PYTHON_PATH),)
 NO_PYTHON = NoThanks
 endif
 
+ifndef PAGER_ENV
+PAGER_ENV = LESS=FRX LV=-c
+endif
+
 QUIET_SUBDIR0  = +$(MAKE) -C # space to separate -C and subdir
 QUIET_SUBDIR1  =
 
@@ -1629,6 +1641,10 @@ ifdef DEFAULT_HELP_FORMAT
 BASIC_CFLAGS += -DDEFAULT_HELP_FORMAT='"$(DEFAULT_HELP_FORMAT)"'
 endif
 
+PAGER_ENV_CQ = "$(subst ",\",$(subst \,\\,$(PAGER_ENV)))"
+PAGER_ENV_CQ_SQ = $(subst ','\'',$(PAGER_ENV_CQ))
+BASIC_CFLAGS += -DPAGER_ENV='$(PAGER_ENV_CQ_SQ)'
+
 ALL_CFLAGS += $(BASIC_CFLAGS)
 ALL_LDFLAGS += $(BASIC_LDFLAGS)
 
@@ -1753,7 +1769,7 @@ common-cmds.h: $(wildcard Documentation/git-*.txt)
 
 SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
$(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\
-   $(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP)
+   $(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV)
 define cmd_munge_script
 $(RM) $@ $@+ && \
 sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
@@ -1766,6 +1782,7 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
 -e 's|@@GITWEBDIR@@|$(gitwebdir_SQ)|g' \
 -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \
 -e 's|@@SANE_TEXT_GREP@@|$(SANE_TEXT_GREP)|g' \
+-e 's|@@PAGER_ENV@@|$(PAGER_ENV_SQ)|g' \
 $@.sh >$@+
 endef
 
@@ -2173,6 +2190,7 @@ GIT-BUILD-OPTIONS: FORCE
@echo NO_PERL=\''$(subst ','\'',$(subst ','\'',$(NO_PERL)))'\' >>$@+
@echo NO_PYTHON=\''$(subst ','\'',$(subst ','\'',$(NO_PYTHON)))'\' >>$@+
@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst 
','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
+   @echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
 ifdef TEST_OUTPUT_DIRECTORY
@echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst 
','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+
 endif
diff --git a/config.mak.uname b/config.mak.uname
index 17fed2f..2484dfb 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -209,6 +209,7 @@ ifeq ($(uname_S),FreeBSD)
HAVE_PATHS_H = YesPlease
GMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes
HAVE_BSD_SYSCTL = YesPlease
+   PAGER_ENV = LESS=FRX LV=-c MORE=-R
 endif
 ifeq ($(uname_S),OpenBSD)
NO_STRCASESTR = YesPlease
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 0c34aa6..0f5a56f 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -163,9 +163,11 @@ git_pager() {
else
GIT_PAGER=cat
fi
-   : "${LESS=-FRX}"
-   : "${LV=-c}"
-   export LESS LV
+   for vardef in @@PAGER_ENV@@
+   do
+   var=${vardef%%=*}
+   eval ": \${$vardef} && export $var"
+   done
 
eval "$GIT_PAGER" '"$@"'
 }
diff --git a/pager.c b/pager.c
index 4bc0481..cd1ac54 100644
--- a/pager.c
+++ b/pager.c
@@ -63,14 +63,35 @@ const char *git_pager(int stdout_is_tty)
return pager;
 }
 
+static void setup_pager_env(struct argv_array *env)
+{
+   const char *pager_env = PAGER_ENV;
+
+   while (*pager_env) {
+   struct strbuf buf = STRBUF_INIT;
+   const char *cp = strchrnul(pager_env, '=');
+
+   if (!*cp)
+   die("malformed build-time PAGER_ENV");
+   strbuf_add(, pager_env, cp - pager_env);
+   cp = strchrnul(pager_env, ' ');
+   if (!getenv(buf.buf)) {
+   strbuf_reset();
+   strbuf_add(, pager_env, cp - pager_env);
+   argv_array_push(env, strbuf_detach(, NULL));
+   }
+   strbuf_reset();
+   while (*cp && *cp == ' ')
+   cp++;
+   pager_env = cp;

[PATCH 0/1 v2] add PAGER_ENV to build

[Eric Wong]

Changes from v1:

* dropped stringify macro in favor for quoting in Makefile
  (diff below)
  I'm not sure I like this change, and might be inclined to
  go in the opposite direction of using the stringify macro
  more widely to simplify the Makefile; but that is a separate
  topic.

* dropped 2/2, I don't have a good rationale for it, either,
  other than "it seemed easy" after 1/2 :>

The following changes since commit f8f7adce9fc50a11a764d57815602dcb818d1816:

  Sync with maint (2016-07-28 14:21:18 -0700)

are available in the git repository at:

  git://bogomips.org/git-svn.git pager-env-v2

for you to fetch changes up to d3aed319c9abac006060bc81e865c93ff8363066:

  pager: move pager-specific setup into the build (2016-08-01 21:46:25 +)


Junio C Hamano (1):
  pager: move pager-specific setup into the build

 Makefile | 20 +++-
 config.mak.uname |  1 +
 git-sh-setup.sh  |  8 +---
 pager.c  | 29 +
 4 files changed, 50 insertions(+), 8 deletions(-)

interdiff from 1/1 v1:

diff --git a/Makefile b/Makefile
index fe469a6..0b36b5e 100644
--- a/Makefile
+++ b/Makefile
@@ -1591,7 +1591,6 @@ PYTHON_PATH_SQ = $(subst ','\'',$(PYTHON_PATH))
 TCLTK_PATH_SQ = $(subst ','\'',$(TCLTK_PATH))
 DIFF_SQ = $(subst ','\'',$(DIFF))
 PERLLIB_EXTRA_SQ = $(subst ','\'',$(PERLLIB_EXTRA))
-PAGER_ENV_SQ = $(subst ','\'',$(PAGER_ENV))
 
 # We must filter out any object files from $(GITLIBS),
 # as it is typically used like:
@@ -1604,7 +1603,7 @@ PAGER_ENV_SQ = $(subst ','\'',$(PAGER_ENV))
 LIBS = $(filter-out %.o, $(GITLIBS)) $(EXTLIBS)
 
 BASIC_CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER_SQ)' \
-   $(COMPAT_CFLAGS) -DPAGER_ENV='$(PAGER_ENV_SQ)'
+   $(COMPAT_CFLAGS)
 LIB_OBJS += $(COMPAT_OBJS)
 
 # Quote for C
@@ -1642,6 +1641,10 @@ ifdef DEFAULT_HELP_FORMAT
 BASIC_CFLAGS += -DDEFAULT_HELP_FORMAT='"$(DEFAULT_HELP_FORMAT)"'
 endif
 
+PAGER_ENV_CQ = "$(subst ",\",$(subst \,\\,$(PAGER_ENV)))"
+PAGER_ENV_CQ_SQ = $(subst ','\'',$(PAGER_ENV_CQ))
+BASIC_CFLAGS += -DPAGER_ENV='$(PAGER_ENV_CQ_SQ)'
+
 ALL_CFLAGS += $(BASIC_CFLAGS)
 ALL_LDFLAGS += $(BASIC_LDFLAGS)
 
diff --git a/pager.c b/pager.c
index 2f2cadc..cd1ac54 100644
--- a/pager.c
+++ b/pager.c
@@ -63,12 +63,9 @@ const char *git_pager(int stdout_is_tty)
return pager;
 }
 
-#define stringify_(x) #x
-#define stringify(x) stringify_(x)
-
 static void setup_pager_env(struct argv_array *env)
 {
-   const char *pager_env = stringify(PAGER_ENV);
+   const char *pager_env = PAGER_ENV;
 
while (*pager_env) {
struct strbuf buf = STRBUF_INIT;
-- 
EW
--
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 1/2] format-patch: Add a config option format.from to set the default for --from

[Junio C Hamano]

Josh Triplett  writes:

> Subject: Re: [PATCH v2 1/2] format-patch: Add a config option format.from ...

At least s/Add/add/; but I would prefer an even shorter

format-patch: format.from gives the default for --from

> +static char *from;

The same "this does not quite help the transition" comment applies
to this one.

> +enum from {
> + FROM_AUTHOR,
> + FROM_USER,
> + FROM_VALUE,

Drop trailing comma after the last enum definition (trailing comma
after the last element in an array is OK, though).

> +static void set_from(enum from type, const char *value)
> +{
> + free(from);
> + switch (type) {
> + case FROM_AUTHOR:
> + from = NULL;
> + break;
> + case FROM_USER:
> + from = xstrdup(git_committer_info(IDENT_NO_DATE));
> + break;
> + case FROM_VALUE:
> + from = xstrdup(value);
> + break;
> + }
> +}

I tend to agree with what Jeff said; I'd queue 1/2 from the original
round for now.

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] reset cached ident date before creating objects

[Junio C Hamano]

Jeff King  writes:

> I also didn't go looking for other spots which might want similar
> treatment. Junio mentioned that the sequencer code still uses an
> external commit process, so cherry-pick/revert are OK. git-fast-import
> creates a stream of commits, but already deals with this issue in a
> separate way. And I couldn't think of other programs that want to make a
> string of commits.

Thanks.  I wonder if do_commit() may be a more appropriate place to
reset this, but the difference only would matter in "am --resolved",
and a call to do_commit() it has will always be the first commit in
the process, so there would not be anything to clear, which would in
turn mean that it would not make any difference.

Will queue.



--
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: [PATCHv3 0/7] submodule update: allow '.' for branch value

[Junio C Hamano]

Stefan Beller  writes:

> This got bigger than expected, but I am happier with the results.
>
> The meat is found in the last patch. (At least what I am interested in; others
> may be more interested in the second patch which could be argued to be a real
> bug fix to be merged down to maint.)

Indeed 2/7 and 7/7 are both interesting.

Will queue; 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: git grep -P is multiline for negative lookahead/behind

[Junio C Hamano]

Michael Giuffrida  writes:

> Is this expected behavior, and if so, why/where is this documented?

I do not think "git grep" was designed to do multi-line anything,
with or without lookahead.  If you imagine that the implementation
attempts its matches line-by-line, does that explain the observed
symptom?
--
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 3/3] t3700: add a test_mode_in_index helper function

[Junio C Hamano]

Ingo Brückl  writes:

Ingo Brückl  writes:

> The case statement to check the file mode of a staged file appears
> a number of times.
>
> Simplify the test by utilizing a test_mode_in_index helper function.
>
> Signed-off-by: Ingo Brückl 
> ---
>  t/t3700-add.sh | 54 ++
>  1 file changed, 22 insertions(+), 32 deletions(-)

Nice.

> diff --git a/t/t3700-add.sh b/t/t3700-add.sh
> index 1fa5dfd..7b98483 100755
> --- a/t/t3700-add.sh
> +++ b/t/t3700-add.sh
> @@ -7,6 +7,20 @@ test_description='Test of git add, including the -- option.'
>
>  . ./test-lib.sh
>
> +# Test the file mode "$1" of the file "$2" in the index.
> +test_mode_in_index () {
> + case "$(git ls-files --stage "$2")" in
> + $1\ *"$2")
> + echo pass
> + ;;
> + *)
> + echo fail
> + git ls-files --stage "$2"
> + return 1
> + ;;
> + esac
> +}

This case/esac is misindented, but no need to resend; I can fix it
up trivially while queuing the patches.  It may be both easier to
read and more robust to tweak the pattern like this, though:

# Test the file mode "$1" of the file "$2" in the index.
test_mode_in_index () {
case "$(git ls-files --stage "$2")" in
"$1 "*" $2")
echo pass
;;
*)
echo fail
git ls-files --stage "$2"
return 1
;;
esac
}

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: Un-paged commit messages in git filter-branch's commit-filter?

[Jeff King]

On Sun, Jul 31, 2016 at 06:39:35PM +0200, Stefan Tauner wrote:

> > There are some output formats that will wrap lines, but by default,
> > filter-branch should not be using them (and I could not reproduce the
> > issue in a simple test). Can you show us what your commit-filter looks
> > like?
> 
> Thanks for your answer. I have tried to reproduce it in other (newly
> created) repositories but failed. However, it seems to relate to some
> kind of persistent paging setting, is that possible?
> git config -l does not show anything suspicious.
> 
> The following commands produce paged output:
> git show hash
> git show --pretty=%B
> git log hash^..hash
> Commit message in gitk
> 
> 
> These do NOT produce paged output:
> git patch hash^..hash
> Commit message in gitg 0.2.7

What is "git patch"? An alias for "format-patch?".

> This is the script I tried to use to reproduce the problem:
> 
> #!/bin/bash
> export LC_ALL=C
> input=$(cat)
> echo "===
> $input
> ===" >> /tmp/paging_bug.txt
> git commit-tree "$@" -m "$input"

Can you be more specific about the input you're feeding to git and the
output you're seeing?

For instance, if I do:

  git init
  echo content >file
  git add file
  git commit -m "$(perl -e 'print join(" ", 1..100)')"

I get a commit message with one long unwrapped line, which I can view
via git-log, etc. Now if I try to run filter-branch on that:

  git filter-branch --commit-filter '
input=$(cat)
{
echo ""
echo $input
echo ""
} >>/tmp/paging_bug.txt
git commit-tree "$@" -m "$input"
  '

then the commit remains unchanged, and paging_bug shows one long line.
What am I missing?

(I wondered at first if the extra "cat" and "-m" could be messing up
whitespace for you, but it should not, as the quoting around "$input"
should preserve things like newlines. And anyway, the bug in that case
would be the _opposite_; I'd expect it to stuff everything onto a single
line rather than breaking lines).

-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: [ANNOUNCE] git-series: track changes to a patch series over time

[Eric Wong]

Josh Triplett  wrote:
> On Mon, Aug 01, 2016 at 07:55:54AM +, Eric Wong wrote:
> > Christian Couder  wrote:
> > > On Fri, Jul 29, 2016 at 12:10 PM, Richard Ipsum
> > >  wrote:
> > > > On Thu, Jul 28, 2016 at 11:40:55PM -0700, Josh Triplett wrote:
> > > > [snip]
> > > >>
> > > >> I'd welcome any feedback, whether on the interface and workflow, the
> > > >> internals and collaboration, ideas on presenting diffs of patch series,
> > > >> or anything else.
> > 
> > > > I'm particularly interested in trying to establish a standard for
> > > > storing review data in git. I've got a prototype for doing that[3],
> > > > and an example tool that uses it[4]. The tool is still incomplete/buggy 
> > > > though.

> > I'm not convinced another format/standard is needed besides the
> > email workflow we already use for git and kernel development.
> 
> Not all projects use a patches-by-email workflow, or want to.  To the
> extent that tools and projects use some other workflow, standardizing
> the format they use to store patch reviews (including per-line
> annotations, approvals, test results, etc) seems preferable to having
> each tool use its own custom format.

I think standardizing on email conventions (such as what we
already do with format-patch, request-pull, S-o-b trailers) would
be a step in this direction and a good step to take.

But yeah, I also hope git adopters can somehow be convinced to
also adopt the workflow that built git itself.

> > I also see the reliance on an after-the-fact search engine
> > (which can be tuned/replaced) as philosophically inline with
> > what git does, too, such as not having rename tracking and
> > doing delayed deltafication.
> 
> Storing review data in git doesn't mean it needs to end up in the
> history of the project itself; it can use after-the-fact annotations on
> a commit.

Right.  So on public-inbox.org/git today, one could search for
after-the-fact annotations based on commit titles and maybe
exact commit ID matches.

A future goal might be to get search indexing working on commit
ID substrings.  So finding references to commit
deadbeefcafe01234567890123467890abcdef00 could be done by
searching for "commit deadbeefcafe" or even a shorter ID, and
the following results could still be returned:

  1. commit deadbeefcafe broke my cat feeder
  2. commit deadbeef killed my cow

> > Email also has the advantage of having existing tooling, and
> > being (at least for now) federated without a single point of
> > failure.
> 
> Storing review data in git makes it easy to push and pull it, which can
> provide the basis for a federated system.

Every public-inbox exposed over HTTP(S) is git clonable[1], so
it's possible to push/pull or have developers merge/combine
inboxes with index-only operations.  There's no UI for that,
yet, and having a working tree checked out is inefficient with
300K uncompressed mails...

But there needs to be way to message others about the existence
of new pushes/pull-requests/reviews/etc; including users
unable to clone or host 800M git repos; so that messaging
system might as well be email.



[1] git clone --mirror https://public-inbox.org/git/
That's not efficient, yet, though, at around 800M when the
gzipped fast-export dump is around half that:
https://public-inbox.org/git/20160710034745.ga20...@dcvr.yhbt.net/T/#u
--
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: Recovering Folder from Git Restored Repo

[Jeff King]

On Mon, Aug 01, 2016 at 04:58:47PM -0400, Dennis Putnam wrote:

> I am in a bit of a pickle. I deleted a remote repo that had a folder
> that I belatedly realized I need. The deleted repo exists on a backup
> which I restored. How do I extract the needed folder from that restored
> repo. Not being a git expert I'm not sure what to do but I cannot find
> any recognizable sources. This is an 'ls' of the restored repo:
> 
> branches  config  description  HEAD  hooks  info  objects  ref

That's a "bare" repository, which has no matching working tree, just the
git data. The simplest thing may be to just clone it; the clone will
have a working tree:

  git clone /path/to/restored-repo tmp
  cd tmp

and then you can pick out whatever files you like.

You can also extract the data straight from the bare repository.  Try
"git ls-tree -r HEAD" to see the complete listing of files (replace HEAD
with the name of a specific branch if it was there). You can extract
individual files with "git show HEAD:path/to/file". Or you can generate
a tar or zip archive of a subtree like:

  git archive --format=zip HEAD:my-subdir >foo.zip

-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] pass constants as first argument to st_mult()

[Junio C Hamano]

Jeff King  writes:

>> -mx = xcalloc(st_mult(num_create, NUM_CANDIDATE_PER_DST), sizeof(*mx));
>> +mx = xcalloc(st_mult(NUM_CANDIDATE_PER_DST, num_create), sizeof(*mx));
>
> I didn't look at all of the calls, but I wonder if it is a natural
> pattern to put the constant second.  

Between

st_mult(GIT_SHA1_RAWSZ, i)
st_mult(i, GIT_SHA1_RAWSZ)

the former is more intuitive at least to me [*1*], but calloc(3) disagrees
with me.

> Since multiplication is
> commutative, it would be correct for st_mult() to just flip the order of
> arguments it feeds to unsigned_mult_overflows().
>
> That may introduce the same inefficiency in other callsites, but I
> wonder if it would be fewer.

"git grep -A3 st_mult \*.c" seems to tell me that the callsites with
a constant in their first parameter are the majority (many are
sizeof(something)).  The three places in the patch under discussion
are the only places that got them in the different order.


[Footnote]

*1* I have a slight suspicion that this is cultural, i.e. how
arithmetic is taught in grade schools.  When an apple costs 30 yen
and I have 5 of them, I was taught to multiply 30x5 to arrive at
150, not 5x30=150, and I am guessing that is because the former
matches the natural order of these two numbers (cost, quantity) in
the language I was taught.
--
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

Recovering Folder from Git Restored Repo

[Dennis Putnam]

I am in a bit of a pickle. I deleted a remote repo that had a folder
that I belatedly realized I need. The deleted repo exists on a backup
which I restored. How do I extract the needed folder from that restored
repo. Not being a git expert I'm not sure what to do but I cannot find
any recognizable sources. This is an 'ls' of the restored repo:

branches  config  description  HEAD  hooks  info  objects  ref

TIA.




signature.asc
Description: OpenPGP digital signature

Re: [PATCH] pass constants as first argument to st_mult()

[Jeff King]

On Mon, Aug 01, 2016 at 02:00:46PM -0700, Junio C Hamano wrote:

> > Since multiplication is
> > commutative, it would be correct for st_mult() to just flip the order of
> > arguments it feeds to unsigned_mult_overflows().
> >
> > That may introduce the same inefficiency in other callsites, but I
> > wonder if it would be fewer.
> 
> "git grep -A3 st_mult \*.c" seems to tell me that the callsites with
> a constant in their first parameter are the majority (many are
> sizeof(something)).  The three places in the patch under discussion
> are the only places that got them in the different order.

Thanks for checking. I should have been less lazy and done it myself.
If the majority are the other way, I agree that just fixing the minority
is the best path forward.

> *1* I have a slight suspicion that this is cultural, i.e. how
> arithmetic is taught in grade schools.  When an apple costs 30 yen
> and I have 5 of them, I was taught to multiply 30x5 to arrive at
> 150, not 5x30=150, and I am guessing that is because the former
> matches the natural order of these two numbers (cost, quantity) in
> the language I was taught.

You might be right. I was trying to figure out what is "natural" for me
in these cases, but after thinking about it for 2 minutes, I'm pretty
sure anything resembling "natural" is lost as I try to out-think myself. :)

-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] t7063: work around FreeBSD's lazy mtime update feature

[Junio C Hamano]

Duy Nguyen  writes:

> On Mon, Aug 1, 2016 at 3:37 AM, Torstem Bögershausen  wrote:
>> the term FREEBSD may be too generic to point out a single feature
>> in an OS distributution.
>> Following your investigations, it may even be possible that
>> other systems adapt this "feature"?
>>
>> How about
>> LAZY_DIR_MTIME_UPDATE
>> (or similar)
>
> This feature was added in 1998, so yes there's a chance it has spread
> to a few fbsd derivatives (not sure if openbsd or netbsd is close
> enough and they ever exchange changes). But I'd rather wait for the
> second OS to expose the same feature before renaming it.

I think a name based on the observed behaviour ("feature") would be
more appropriate because I'd be more worried about us finding other
glitches we see (initially) only on FBSD.  People who need to adjust
tests that use the same FBSD prereq would have to wonder which
prereq-skip is due to which glitch.
--
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] gitweb: escape link body in format_ref_marker

[Junio C Hamano]

Jakub Narębski  writes:

> Good catch!
>
> Acked-by: Jakub Narębski 

Sigh; the contents may be good but the patch is unusable as-is
because of heavy whitespace damage.

I'll fix it up.  Thanks, both.

>> ---
>>  gitweb/gitweb.perl | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index 2fddf75..33d701d 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -2090,7 +2090,7 @@ sub format_ref_marker {
>> -href => href(
>> action=>$dest_action,
>> hash=>$dest
>> -   )}, $name);
>> +   )}, esc_html($name));
>> 
>> $markers .= " > class=\"".esc_attr($class)."\" title=\"".esc_attr($ref)."\">" .
>> $link . "";
>> 
--
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] use strbuf_addstr() for adding constant strings to a strbuf

[Junio C Hamano]

Jeff King  writes:

> On Sat, Jul 30, 2016 at 07:36:23PM +0200, René Scharfe wrote:
>
>> Replace uses of strbuf_addf() for adding strings with more lightweight
>> strbuf_addstr() calls.
>> 
>> In http-push.c it becomes easier to see what's going on without having
>> to verfiy that the definition of PROPFIND_ALL_REQUEST doesn't contain
>> any format specifiers.
>
> Nice. I care a lot less about whether "addf" or "addstr" is more
> efficient. But the second point, that it makes the intent clearer, is a
> big win.

Yes!
--
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 v9 33/41] environment: add set_index_file()

[Junio C Hamano]

Junio C Hamano  writes:

> Stefan Beller  writes:
>
>> In cache.h we have a NO_THE_INDEX_COMPATIBILITY_MACROS,
>> and lots of
>>   #define foo_bar(..) frob_bar(_index, (..))
>>
>> Could you operate on the raw functions that take pointers to _index
>> and point these to a temporary index?
>
> Isn't mention of the_index is a red-herring?
>
> The in-core index_state does not even know what file it needs to be
> written to, so whether you explicitly specify your own index or use
> the compat macros to access the_index, you would need to specify to
> which file you would write it out or from which file you would read
> the new contents.

Having said that, I agree with you that the cop-out "Yes we know
this is bad" needs a lot more clarification, pointing out what issue
this side-steps and a direction to solve it correctly.
--
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] format-patch: Add a config option format.from to set the default for --from

[Junio C Hamano]

Josh Triplett  writes:

> +static char *from;
>  static const char *signature = git_version_string;
>  static const char *signature_file;
>  static int config_cover_letter;
> @@ -807,6 +808,17 @@ static int git_format_config(const char *var, const char 
> *value, void *cb)
>   base_auto = git_config_bool(var, value);
>   return 0;
>   }
> + if (!strcmp(var, "format.from")) {
> + int b = git_config_maybe_bool(var, value);
> + free(from);
> + if (b < 0)
> + from = xstrdup(value);
> + else if (b)
> + from = xstrdup(git_committer_info(IDENT_NO_DATE));
> + else
> + from = NULL;
> + return 0;
> + }

One potential issue I see here is that if we ever plan to switch the
default, we may want to issue a warning message to users who do not
have any format.from configured when they do run the program on a
commit that will get a different result before and after the switch
in a release of Git before that default switch happens.  The message
would say something like "you are formatting somebody else's commit.
the output will change in future versions of Git and show an explicit
in-body From: header; if you want to keep the current behaviour, set
format.from configuration variable to false".

But you cannot tell by looking at from that is NULL between two
cases, it is NULL because we defaulted to it (in which case we do
want to warn), or the user set it explicitly to false (we do not
want to give the warning).

So "... makes it easier to change the default in the future." in the
log message is a bit of exaggeration.  The change in this patch is
not there yet.

> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index 1206c48..b0579dd 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -229,6 +229,46 @@ check_patch () {
>   grep -e "^Subject:" "$1"
>  }
>  
> +test_expect_success 'format.from=false' '
> +
> + git -c format.from=false format-patch --stdout master..side |
> + sed -e "/^\$/q" >patch &&
> + check_patch patch &&
> + ! grep "^From: C O Mitter \$" patch

I know you are only mimicking the style of the existing tests but
the piping loses a potential error exit code from format-patch.  I'd
suggest leaving this as low-hanging fruit for later, not fixing them
as part of this series.

This stops at the blank after the header, so there is no point doing
master..side to format three patches.  Just do "-1 side" instead?

More importantly, this only checks the From: in the header, which is
just the half of what --from does.  Shouldn't we be testing that the
in-body From: is added as necessary?

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 v9 33/41] environment: add set_index_file()

[Junio C Hamano]

Stefan Beller  writes:

> In cache.h we have a NO_THE_INDEX_COMPATIBILITY_MACROS,
> and lots of
>   #define foo_bar(..) frob_bar(_index, (..))
>
> Could you operate on the raw functions that take pointers to _index
> and point these to a temporary index?

Isn't mention of the_index is a red-herring?

The in-core index_state does not even know what file it needs to be
written to, so whether you explicitly specify your own index or use
the compat macros to access the_index, you would need to specify to
which file you would write it out or from which file you would read
the new contents.

--
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] reset cached ident date before creating objects

[Jeff King]

On Mon, Aug 01, 2016 at 02:00:47PM -0400, Jeff King wrote:

> So at this point I think I'd lean towards programs which have multiple
> logical commit operations explicitly saying "I am starting a new
> operation". It may be that we end up attaching more to that in the long
> run than just timestamp resets, too (i.e., what other cached data might
> there be that used to happen in separate sub-processes?).

Here's that patch.

I still just put in a bare "reset_ident_date()" in git-am. This _could_
be wrapped in: begin_logical_commit() or something, but it would just be
a wrapper around resetting the ident_date. So I'm inclined not to worry
about such semantic obfuscation until we actually have a second thing to
reset. :)

I also didn't go looking for other spots which might want similar
treatment. Junio mentioned that the sequencer code still uses an
external commit process, so cherry-pick/revert are OK. git-fast-import
creates a stream of commits, but already deals with this issue in a
separate way. And I couldn't think of other programs that want to make a
string of commits.

-- >8 --
Subject: [PATCH] am: reset cached ident date for each patch

When we compute the date to go in author/committer lines of
commits, or tagger lines of tags, we get the current date
once and then cache it for the rest of the program.  This is
a good thing in some cases, like "git commit", because it
means we do not racily assign different times to the
author/committer fields of a single commit object.

But as more programs start to make many commits in a single
process (e.g., the recently builtin "git am"), it means that
you'll get long strings of commits with identical committer
timestamps (whereas before, we invoked "git commit" many
times and got true timestamps).

This patch addresses it by letting callers reset the cached
time, which means they'll get a fresh time on their next
call to git_committer_info() or git_author_info(). The first
caller to do so is "git am", which resets the time for each
patch it applies.

It would be nice if we could just do this automatically
before filling in the ident fields of commit and tag
objects. Unfortunately, it's hard to know where a particular
logical operation begins and ends.

For instance, if commit_tree_extended() were to call
reset_ident_date() before getting the committer/author
ident, that doesn't quite work; sometimes the author info is
passed in to us as a parameter, and it may or may not have
come from a previous call to ident_default_date(). So in
those cases, we lose the property that the committer and the
author timestamp always match.

You could similarly put a date-reset at the end of
commit_tree_extended(). That actually works in the current
code base, but it's fragile. It makes the assumption that
after commit_tree_extended() finishes, the caller has no
other operations that would logically want to fall into the
same timestamp.

So instead we provide the tool to easily do the reset, and
let the high-level callers use it to annotate their own
logical operations.

There's no automated test, because it would be inherently
racy (it depends on whether the program takes multiple
seconds to run). But you can see the effect with something
like:

  # make a fake 100-patch series
  top=$(git rev-parse HEAD)
  bottom=$(git rev-list --first-parent -100 HEAD | tail -n 1)
  git log --format=email --reverse --first-parent \
  --binary -m -p $bottom..$top >patch

  # now apply it; this presumably takes multiple seconds
  git checkout --detach $bottom
  git am 
Helped-by: Paul Tan 
Signed-off-by: Jeff King 
---
 builtin/am.c | 2 ++
 cache.h  | 1 +
 ident.c  | 5 +
 3 files changed, 8 insertions(+)

diff --git a/builtin/am.c b/builtin/am.c
index b77bf11..8aa9b5b 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1840,6 +1840,8 @@ static void am_run(struct am_state *state, int resume)
const char *mail = am_path(state, msgnum(state));
int apply_status;
 
+   reset_ident_date();
+
if (!file_exists(mail))
goto next;
 
diff --git a/cache.h b/cache.h
index b5f76a4..31e65f9 100644
--- a/cache.h
+++ b/cache.h
@@ -1269,6 +1269,7 @@ extern const char *ident_default_email(void);
 extern const char *git_editor(void);
 extern const char *git_pager(int stdout_is_tty);
 extern int git_ident_config(const char *, const char *, void *);
+extern void reset_ident_date(void);
 
 struct ident_split {
const char *name_begin;
diff --git a/ident.c b/ident.c
index 139c528..e20a772 100644
--- a/ident.c
+++ b/ident.c
@@ -184,6 +184,11 @@ static const char *ident_default_date(void)
return git_default_date.buf;
 }
 
+void reset_ident_date(void)
+{
+   strbuf_reset(_default_date);
+}
+
 static int crud(unsigned char c)
 {
return  c <= 32  ||
-- 
2.9.2.670.gf6ce898

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

Re: git bisect for reachable commits only

[Christian Couder]

On Mon, Aug 1, 2016 at 9:51 PM, Junio C Hamano  wrote:
> Christian Couder  writes:
>
>> Yes, and the reason is that all the ancestors of a good commit are
>> considered good.
>> That's because git bisect supposes that there is only one transition
>> from good to bad.
>> Otherwise we would need a more complex algorithm to find all the
>> transitions from good to bad, and that is not generally needed.
>
> It may be debatable if that is generally needed or not, but by
> definition "bisect" is about having a history that has a SINGLE
> point that changes from good to bad (or old to new, or "have sugar"
> to "no sugar"), and that single-ness is what allows us to BIsect the
> graph.  So even if it may be a good thing to have to be able to find
> multiple transitions, that is outside the scope of how the current
> "git bisect" was designed to be used.

Yeah, this is a better version of what I wanted to say.

>> I haven't looked at your previous issue much, sorry I have been busy these 
>> days.
>
> I think the "previous issue" was that we can ask the user to ask to
> check one of 'x' or 'y' when given Good and Bad points in a graph like
> this:
>
> x---y---y---o---B
>  \ /
>   x---G---o
>
> while a more natural expectation by a user would be that we only
> need to check one of these two 'o'.

Yeah, I reproduced the steps described in the Google Groups discussion:

https://groups.google.com/forum/#!topic/git-users/v3__t42qbKE

and I think that is indeed the "previous issue".

> Thinking about it again, I actually think it makes sense to ask the
> user to check 'y'; there is no good reason to believe that 'y' can
> never have introduced the badness we are hunting for, and limiting
> the search to --ancestry-path (which is to ask only for 'o') would
> stop at the merge 'o' if one of the 'y' were bad, which would
> prevents us from knowing the exact breakage.

I agree.

> There however is no excuse if we asked to check 'x', though.  They
> are ancestors of a Good commit, and "git bisect" should be able to
> assume they are Good.

I think it does. When I reproduced the steps in the "previous issue",
it did assume they are good.
--
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/4] rebase: avoid computing unnecessary patch IDs

[Junio C Hamano]

Johannes Schindelin  writes:

> Hi Junio,
>
> On Fri, 29 Jul 2016, Junio C Hamano wrote:
>
>> Kevin Willford  writes:
>> 
>> >  static int patch_id_cmp(struct patch_id *a,
>> >struct patch_id *b,
>> > -  void *keydata)
>> > +  struct diff_options *opt)
>> >  {
>> > +  if (is_null_sha1(a->patch_id) &&
>> > +  commit_patch_id(a->commit, opt, a->patch_id, 0))
>> > +  return error("Could not get patch ID for %s",
>> > +  oid_to_hex(>commit->object.oid));
>> > +  if (is_null_sha1(b->patch_id) &&
>> > +  commit_patch_id(b->commit, opt, b->patch_id, 0))
>> > +  return error("Could not get patch ID for %s",
>> > +  oid_to_hex(>commit->object.oid));
>> >return hashcmp(a->patch_id, b->patch_id);
>> >  }
>> 
>> These error returns initially looks slightly iffy in that in general
>> the caller of any_cmp_fn() wants to know how a/b compares, but by
>> returning error(), it always says "a is smaller than b".
>
> I am to blame, as this is my design.
>
> And yes, it is kind of funny that we require a cmpfn that returns <0, ==0
> and >0 for comparisons, when hashmaps try to avoid any order.

Perhaps hashmap API needs fixing in the longer term not to call this
type hashmap_cmp_fn; instead it should lose cmp and say something
like hashmap_eq_fn or something.

> Do you want a note in the commit message about this "abuse" of a negative
> return value, or a code comment?

I do not think negative (or non-zero) return is an "abuse" at all.
It is misleading in the context of the function whose name has "cmp"
in it, but that is not the fault of this function, rather, the
breakage is more in the API that calls a function that wants to know
only equality a "cmp".  A in-code comment before the function name
may be appropriate:

/*
 * hashmap API calls hashmap_cmp_fn, but it only wants
 * "does the key match the entry?" with 0 (matches) and
 * non-zero (does not match).
 */
static int patch_id_match(const struct patch_id *ent,
  const struct patch_id *key,
  const void *keydata)
{
...

--
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] 1/4] patch-ids: stop using a hand-rolled hashmap implementation

[Junio C Hamano]

Johannes Schindelin  writes:

> It would be a serious bug if hashmap_entry_init() played games with
> references, given its signature (that this function does not have any
> access to the hashmap structure, only to the entry itself):
>
>   void hashmap_entry_init(void *entry, unsigned int hash)

I do not think we are on the same page.  The "reference to other
resource" I wondered was inside the hashmap_entry structure, IOW,
"the entry itself".

Which is declared to be opaque to the API users, so whoever defined
that API cannot blame me for not checking its definition to see that
it only has "unsigned int hash" and no allocated memory or open file
descriptor in it that needs freeing.

By the way, the first parameter of the function being "void *" is
merely to help lazy API users, who have their own structure that
embeds the hashmap_entry as its first element, as API documentation
tells them to do, e.g.

struct foo {
struct hashmap_entry e;
... other "foo" specific fields come here ...
} foo;

and because of the lazy "void *", they do not have to do this:

hashmap_entry_init(>e, ...);

which would be required if the first parameter were "struct
hashmap_entry *", but they can just do this:

hashmap_entry_init(, ...);

I have a slight preference to avoid the lazy "void *", but that is
an unrelated tangent.

>> The fact that hashmap_entry_init() is there but there is no
>> corresponding hashmap_entry_clear() hints that there is nothing to
>> be worried about and I can see from the implementation of
>> hashmap_entry_init() that no extra resource is held inside, but an
>> API user should not have to guess.  We may want to do one of the two
>> things:
>> 
>>  * document that an embedded hashmap_entry does not hold any
>>resource that need to be released and it is safe to free the user
>>structure that embeds one; or
>> 
>>  * implement hashmap_entry_clear() that currently is a no-op.
>
> Urgh. The only reason we have hashmap_entry_init() is that we *may* want
> to extend `struct hashmap_entry` at some point. That is *already*
> over-engineered because that point in time seems quite unlikely to arrive,
> like, ever.

I am saying that an uneven over-enginnering is bad.

To be consistent with having _init() and declaring the entry
structure to be opaque, you would either need _clear() or document
you would never need to worry about the lack of _clear().
--
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 0/2] format-patch: Transition the default to --from to avoid spoofed mails

[Josh Triplett]

On Mon, Aug 01, 2016 at 01:47:24PM -0400, Jeff King wrote:
> On Sat, Jul 30, 2016 at 12:11:05PM -0700, Josh Triplett wrote:
> 
> > Josh Triplett (2):
> >   format-patch: Add a config option format.from to set the default for 
> > --from
> >   format-patch: Default to --from
> 
> By the way, I notice that the threading between your patches and cover
> letter are broken. Since I see you are also working on a tool for
> handling such things, I'd suspect the tool (or your workflow) has a bug.
> :)

My workflow, fortunately. :)

> The message-id of this message is:
> 
>   <20160730191104.2ps5k7eji7aqgufg@x>
> 
> but the patches have both "References" and "In-Reply-To" set to:
> 
>   
> 
> 
> I also see your MUA is mutt, and I think I saw you mention using "mutt
> -H" elsewhere. IIRC, when I started using a similar workflow years ago,
> I tried the same thing and had the same problem: "-H" treats the input
> file as a template, not a message, and thus generates a new message-id.
> 
> I switched to using mutt's internal "resend-message" function, which
> does a more literal re-send. I don't think I ever found a way to
> convince mutt to do a resend from the command line.

I actually tried using mutt's resend function (alt-e) with an mbox; I
checked the Message-Id and In-Reply-To headers, and they looked correct.
I've used mutt -H successfully before without breaking threads.  The
Debian mutt packages recently upgraded to the "neomutt" fork; I wonder
if something broke recently?

Thanks for letting me know; I'll investigate and try to figure out the
problem.

- Josh Triplett
--
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 bisect for reachable commits only

[Junio C Hamano]

Christian Couder  writes:

> Yes, and the reason is that all the ancestors of a good commit are
> considered good.
> That's because git bisect supposes that there is only one transition
> from good to bad.
> Otherwise we would need a more complex algorithm to find all the
> transitions from good to bad, and that is not generally needed.

It may be debatable if that is generally needed or not, but by
definition "bisect" is about having a history that has a SINGLE
point that changes from good to bad (or old to new, or "have sugar"
to "no sugar"), and that single-ness is what allows us to BIsect the
graph.  So even if it may be a good thing to have to be able to find
multiple transitions, that is outside the scope of how the current
"git bisect" was designed to be used.

> I haven't looked at your previous issue much, sorry I have been busy these 
> days.

I think the "previous issue" was that we can ask the user to ask to
check one of 'x' or 'y' when given Good and Bad points in a graph like
this:

x---y---y---o---B
 \ / 
  x---G---o

while a more natural expectation by a user would be that we only
need to check one of these two 'o'.

Thinking about it again, I actually think it makes sense to ask the
user to check 'y'; there is no good reason to believe that 'y' can
never have introduced the badness we are hunting for, and limiting
the search to --ancestry-path (which is to ask only for 'o') would
stop at the merge 'o' if one of the 'y' were bad, which would
prevents us from knowing the exact breakage.

There however is no excuse if we asked to check 'x', though.  They
are ancestors of a Good commit, and "git bisect" should be able to
assume they are Good.



--
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: [ANNOUNCE] git-series: track changes to a patch series over time

[Josh Triplett]

On Mon, Aug 01, 2016 at 09:14:54AM -0600, Stephen Warren wrote:
> On 07/29/2016 12:40 AM, Josh Triplett wrote:
> > I'd like to announce a project I've been working on for a while:
> > 
> > git-series provides a tool for managing patch series with git, tracking
> > the "history of history". git series tracks changes to the patch series
> > over time, including rebases and other non-fast-forwarding changes. git
> > series also tracks a cover letter for the patch series, formats the
> > series for email, and prepares pull requests.
> 
> Just as an FYI, I wouldn't be surprised if there's some overlap, or
> potential for merging of tools, between this tool and the "patman" tool
> that's part of the U-Boot source tree:
> 
> http://git.denx.de/?p=u-boot.git;a=blob;f=tools/patman/README;h=e36857dedea1d0dbafa41732aaf9bf0988d63f38;hb=HEAD

Interesting tool; thanks for the link.

As far as I can tell from that documentation, patman doesn't track old
versions of a patch series; you rebase to modify patches or change
patman tags (embedded in commit messages), and nothing preserves the
previous version.  And it tracks the cover letter and similar in one of
the commit messages in the series, so previous versions of that don't
get saved either.  If you wanted to track the history of your changes,
you'd have to use branch names or similar.

In addition, tracking metadata in commit messages only works with a
patches-by-mail workflow where the messages get processed when
generating patches; that doesn't work for please-pull workflows.

patman does have quite a few interesting ideas, though.  git-series
needs some way of handling To/Cc addresses for patches and the cover
letter (beyond just scripts/get_maintainer.pl), and more automatic
handling of series versioning (v2, v3, ...) and associated series
changelogs.  Suggestions welcome.

- Josh Triplett
--
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 v5 14/16] merge-recursive: offer an option to retain the output in 'obuf'

[Junio C Hamano]

Johannes Schindelin  writes:

>> I actually now see how this would work well for "reason 2)".  If a
>> caller wants to run the function and wants to pretend as if it did
>> not run anything when it failed, for example, using this to spool
>> all output and error to a strbuf and discard it when the function
>> returns an error, and emit the spooled output to standard output and
>> standard error in the order the lines were collected when the
>> function returns a success, would be a good way to do so.
>
> That is actually the exact opposite of the intended usage: when any `pick`
> in an interactive rebase succeeds, its output is discarded so as not to
> bother the user. We show the complete output only when it fails.

Oh, it makes sense, too, to show the output only when there is an
error.

But in that case, there would be both messages meant for the
standard output and also meant for the standard error, and we need
some way to make sure they go to the right channel.

I however do not think an array of  is the only
way to achieve that.  We can get away by a single strbuf that
accumulates all output() that we have seen so far, i.e. "we only
accumulate output() and ignore flush() as long as what we see are
only from output()" mode.

Then the err() routine operating under this new mode can show what
has been accumulated to the standard output (because with this tweak
I am outlining here, by definition, the strbuf will only keep the
output() material and not err() things), show the err() message, and
switch back to the normal "we accumulate output() and honor flush()"
mode.  Of course, when we are doing multiple rounds, the mode must
be reset to "accumulate output and ignore flush" mode at the
beginning of each rouhd.

That would give us "silence if there is no error, but if we are
showing error, show them to the standard error, while giving
non-error message to the standard output".
--
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 v6 05/16] Prepare the builtins for a libified merge_recursive()

[Junio C Hamano]

Johannes Schindelin  writes:

> 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

I think I typofixed this when I queued the previous one on 'pu'
already, but s/synch/sync/; 

--
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] pager: move pager-specific setup into the build

[Duy Nguyen]

On Mon, Aug 1, 2016 at 7:52 PM, Jeff King  wrote:
> On Mon, Aug 01, 2016 at 07:46:34PM +0200, Duy Nguyen wrote:
>
>> On Mon, Aug 1, 2016 at 3:05 AM, Eric Wong  wrote:
>> > From: Junio C Hamano 
>> >
>> > Allowing PAGER_ENV to be set at build-time allows us to move
>> > pager-specific knowledge out of our build.  Currently, this
>> > allows us to set a better default for FreeBSD where more(1)
>> > is the same binary as less(1).
>>
>> Nice. I was just too lazy to do something like this and "export
>> PAGER=less LESS=FRX" then ignored it :-P
>>
>> Slightly off topic, but pagers like 'more' does not understand colors
>> either. But color.ui = auto does not know what and prints color code
>> anyway. It would be nice if we had some configuration to describe
>> "this pager can show colors, that pager does not" so I don't have to
>> maintain separate .gitconfig files on two platforms.
>
> If you are interested, I suggest you read the thread linked earlier:
>
>   https://public-inbox.org/git/52D87A79.6060600%40rawbw.com/T/#u
>
> which discusses this and other issues. But basically, I think you cannot
> really solve this without getting intimate with each pager (which people
> seemed not to want to do).

Cooking pager specifics in git does sound bad. But it does not have to
be that way. What if we delegate the decision whether to color or not
to a script (e.g. by setting color.ui= "script ")?
The script has all the info (env variables, uname, user preference...)
and can make a better decision than 'is stdout a tty?'. It's not about
out of the box experience, more towards customization (without
fragmenting .gitconfig files too much).
-- 
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] reset cached ident date before creating objects

[Jeff King]

On Mon, Aug 01, 2016 at 10:49:12AM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> >> So maybe we would have to put reset_ident_date() at the end of the
> >> function instead, at least after git_committer_info() is called.
> >
> > Yes, although "reset and end" still feels a bit weird to me.
> >
> > I'd almost prefer to just have long-running programs insert resets at
> > strategic points.
> 
> Certainly "reset at the end" feels weird but it can be explained as
> "for a one-shot thing we use the first time of the default date and
> it gives a consistent timestamp; conceptually, things that make
> multiple commits are like doing that one-shot thing multiple times
> in a row."
> 
> When viewed that way, it is not _too_ bad, I would guess.

I think what bothers me is that you could use similar logic for "reset
at the beginning". The problem is that we don't know when "the
beginning" is. I thought it was inside commit_tree_extended(), but for
some operations, it's not, as Paul showed.

So when is "the end"? Is it at the end of commit_tree_extended()? I'm
not sure. Already we know it depends on whether you consider the ref
update part of the same operation. Whether that matters is debatable.
Are there other cases (an obvious one would be the human-readable bits
printed by git-commit, but it looks like those are done beforehand)?
Even if we do check every case, though, it seems like a fragile
assumption to be making.

So at this point I think I'd lean towards programs which have multiple
logical commit operations explicitly saying "I am starting a new
operation". It may be that we end up attaching more to that in the long
run than just timestamp resets, too (i.e., what other cached data might
there be that used to happen in separate sub-processes?).

-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] git-format-patch: default to --from to avoid spoofed mails?

[Junio C Hamano]

Jeff King  writes:

> I half-agree. Config that causes unpredictable behavior can break
> somebody else's script that you are running. If you say "oh, I guess I
> shouldn't set that config" and move on with your life, then the config
> hasn't really hurt anybody. If you complain to the script author that
> their script is broken, and insist that they pass the --no-foo option,
> then the script writer does not care much whether it was a config option
> or a change of default.

Hmph, I never considered that POV but you are right.

I am not sure if that argues that we have to be even more careful
not to add non-essential configuration variable, or that we can
afford to be much less careful when changing the default, though.
A little bit of both, I guess.

--
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 v6 06/16] merge_recursive: abort properly upon errors

[Junio C Hamano]

Johannes Schindelin  writes:

> Subject: Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors

s/merge_/merge-/; for this one alone.

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

s/wie/we/;
--
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 v5 16/16] merge-recursive: flush output buffer even when erroring out

[Junio C Hamano]

Johannes Schindelin  writes:

>> This is of course a good change, but we need to assume that no
>> further output is made from the remainder of the function for the
>> change in the next hunk to remove the existing flush to be correct.
> ...
> But you made me realize that I cannot simply *move* the flush_output()
> call here, in case that code in between will eventually add output.

Yup, that removal of the original one was the only thing I was
pointing out.


--
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: Teach --use-bitmap-index codepath to respect --local, --honor-pack-keep and --incremental

[Junio C Hamano]

Kirill Smelkov  writes:

> Since 6b8fda2d (pack-objects: use bitmaps when packing objects) there
> are two codepaths in pack-objects: with & without using bitmap
> reachability index.
>
> However add_object_entry_from_bitmap(), despite its non-bitmapped
> counterpart add_object_entry(), in no way does check for whether --local
> or --honor-pack-keep or --incremental should be respected. In
> non-bitmapped codepath this is handled in want_object_in_pack(), but
> bitmapped codepath has simply no such checking at all.
>
> The bitmapped codepath however was allowing to pass in all those options
> and with bitmap indices still being used under such conditions -
> potentially giving wrong output (e.g. including objects from non-local or
> .keep'ed pack).
>
> We can easily fix this by noting the following: when an object comes to
> add_object_entry_from_bitmap() it can come for two reasons:
>
> 1. entries coming from main pack covered by bitmap index, and
> 2. object coming from, possibly alternate, loose or other packs.
>
> For "2" we always have pack not yet found by bitmap traversal code, and
> thus we can simply reuse non-bitmapped want_object_in_pack() to find in
> which pack an object lives and also for taking omitting decision.
>
> For "1" we always have pack already found by bitmap traversal code and we
> only need to check that pack for same criteria used in
> want_object_in_pack() for found_pack.
>
> Suggested-by: Junio C Hamano 
> Discussed-with: Jeff King 
> ---

I do not think I suggested much of this to deserve credit like this,
though, as I certainly haven't thought about the pros-and-cons
between adding the same "some object in pack may not want to be in
the output" logic to the bitmap side, or punting the bitmap codepath
when local/keep are involved.

> +/* Like want_object_in_pack() but for objects coming from-under bitmapped 
> traversal */
> +static int want_object_in_pack_bitmap(const unsigned char *sha1,
> +   struct packed_git **found_pack,
> +   off_t *found_offset)
> +{
> + struct packed_git *p = *found_pack;
> +
> + /*
> +  * There are two types of requests coming here:
> +  * 1. entries coming from main pack covered by bitmap index, and
> +  * 2. object coming from, possibly alternate, loose or other packs.
> +  *
> +  * For "1" we always have *found_pack != NULL passed here from
> +  * traverse_bitmap_commit_list(). (*found_pack is bitmap_git.pack
> +  * actually).
> +  *
> +  * For "2" we always have *found_pack == NULL passed here from
> +  * traverse_bitmap_commit_list() - since this is the way bitmap
> +  * traversal passes here "extended" bitmap entries.
> +  */
> +
> + /* objects not covered by bitmap */
> + if (!p)
> + return want_object_in_pack(sha1, 0, found_pack, found_offset);
> + /* objects covered by bitmap - we only have to check p wrt local and 
> .keep */

I am assuming that p != NULL only means "this object exists in THIS
pack", without saying anything about "this object may also exist in
other places", but "we only have to check" implies that "p != NULL"
means "this object exists *ONLY* in this pack and nowhere else".

Puzzled.


> + if (incremental)
> + return 0;
> + if (local && !p->pack_local)
> + return 0;
> + if (ignore_packed_keep && p->pack_local && p->pack_keep)
> + return 0;
> +
> + return 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] reset cached ident date before creating objects

[Jeff King]

On Mon, Aug 01, 2016 at 02:12:34PM -0400, Jeff King wrote:

> Before I switched to "reset at the beginning" in my original patch, I
> also noticed this issue, but decided the other way: to only reset after
> a successful creation.
> 
> I think in most cases it wouldn't matter anyway, because the caller will
> generally abort as soon as this returns failure anyway. But I wondered
> about something like:
> 
>   author = prepare_author_info();
>   if (commit_tree_extended(..., author, ...) < 0) {
>   /* oops, we failed. Do a thing and try again. */
>   possible_fix();
>   if (commit_tree_extended(..., author, ...) < 0)
>   die("giving up");
>   }
> 
> In the second call (but only the second call!) the committer and author
> can diverge.

To be clear, I checked all of the callers and nobody actually does this.
Every caller proceeds straight to a die() except the one in
notes_cache_write(), which silently ignores error (which is the correct
thing to do).

This is more "it seems like a fragile pattern to me".

-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 1/2] pager: move pager-specific setup into the build

[Junio C Hamano]

Jeff King  writes:

> I'm not too worried about spaces here. This is a resurrection of an old
> discussion, and in all that time, I think the only realistic suggestions
> for built-in values have been pretty tame.
>
> If this were used to parse arbitrary user-provided runtime values, I'd
> be more concerned. But I'm not sure why we would need that. Your $EDITOR
> example is arbitrary shell code, and we let the shell handle it (modulo
> some efficiency shortcuts). Likewise, fancy runtime things should go in
> GIT_PAGER, where you can not only set options with spaces, but do fancy
> things like pipes, shell functions, etc.
>
> The use of stringify() here is funny to me; I think there is a cpp
> tokenizing step in the middle that will do things like gobble up
> whitespace (but I'm not sure if it has other possible effects). I think
> our more usual method here would be to C-quote in the Makefile (with the
> equivalent of 's/\\//g; s/"/\\"/g'), and then pass it to the
> compiler as a string literal, like -DPAGER_ENV=\"$(PAGER_ENV_CQ_SQ\".

All sensible arguments, including the rationale to reject 2/2.

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 v3 10/10] convert: add filter..process option

[Lars Schneider]

> On 01 Aug 2016, at 00:19, Jakub Narębski  wrote:
> 
> W dniu 30.07.2016 o 01:38, larsxschnei...@gmail.com pisze:
> [...]
>> +Please note that you cannot use an existing filter..clean
>> +or filter..smudge command as filter..process
>> +command.
> 
> I think it would be more readable and easier to understand to write:
> 
>  ... you cannot use an existing ... command with
>  filter..process
> 
> About the style: wouldn't `filter..process` be better?

OK, changed it!


>> As soon as Git would detect a file that needs to be
>> +processed by this filter, it would stop responding.
> 
> This is quite convoluted, and hard to understand.  I would say
> that because `clean` and `smudge` filters are expected to read
> first, while Git expects `process` filter to say first, using
> `clean` or `smudge` filter without changes as `process` filter
> would lead to git command deadlocking / hanging / stopping
> responding.

How about this:

Please note that you cannot use an existing `filter..clean`
or `filter..smudge` command with `filter..process`
because the former two use a different inter process communication
protocol than the latter one. As soon as Git would detect a file
that needs to be processed by such an invalid "process" filter, 
it would wait for a proper protocol handshake and appear "hanging".


>> +
>> +
>> Interaction between checkin/checkout attributes
>> ^^^
>> 
>> diff --git a/convert.c b/convert.c
>> index 522e2c5..be6405c 100644
>> --- a/convert.c
>> +++ b/convert.c
>> @@ -3,6 +3,7 @@
>> #include "run-command.h"
>> #include "quote.h"
>> #include "sigchain.h"
>> +#include "pkt-line.h"
>> 
>> /*
>>  * convert.c - convert a file when checking it out and checking it in.
>> @@ -481,11 +482,355 @@ static int apply_filter(const char *path, const char 
>> *src, size_t len, int fd,
>>  return ret;
>> }
>> 
>> +static int multi_packet_read(int fd_in, struct strbuf *sb, size_t 
>> expected_bytes, int is_stream)
> 
> About name of this function: `multi_packet_read` is fine, though I wonder
> if `packet_read_in_full` with nearly the same parameters as `packet_read`,
> or `packet_read_till_flush`, or `read_in_full_packetized` would be better.

I like `multi_packet_read` and will rename!


> Also, the problem is that while we know that what packet_read() stores
> would fit in memory (in size_t), it is not true for reading whole file,
> which might be very large - for example huge graphical assets like raw
> images or raw videos, or virtual machine images.  Isn't that the goal
> of git-LFS solutions, which need this feature?  Shouldn't we have then
> both `multi_packet_read_to_fd` and `multi_packet_read_to_buf`,
> or whatever?

Git LFS works well with the current clean/smudge mechanism that uses the
same on in memory buffers. I understand your concern but I think this
improvement is out of scope for this patch series.


> Also, if we have `fd_in`, then perhaps `sb_out`?

Agreed!


> I am also unsure if `expected_bytes` (or `expected_size`) should not be
> just a size hint, leaving handing mismatch between expected size and
> real size of output to the caller; then the `is_stream` would be not
> needed.

As mentioned in a previous email... I will drop the "size" support in
this patch series as it is not really needed.


>> +{
>> +int bytes_read;
>> +size_t total_bytes_read = 0;
> 
> Why `bytes_read` is int, while `total_bytes_read` is size_t? Ah, I see
> that packet_read() returns an int.  It should be ssize_t, just like
> read(), isn't it?  But we know that packet size is limited, and would
> fit in an int (or would it?).

Yes, it is limited but I agree on ssize_t!


> Also, total_bytes_read could overflow size_t, but then we would have
> problems storing the result in strbuf.

Would that check be ok?

if (total_bytes_read > SIZE_MAX - bytes_read)
return 1;  // `total_bytes_read` would overflow and is 
not representable


>> +if (expected_bytes == 0 && !is_stream)
>> +return 0;
> 
> So in all cases *except* size = 0 we expect flush packet after the
> contents, but size = 0 is a corner case without flush packet?

I agree that is inconsistent... I will change it!


>> +
>> +if (is_stream)
>> +strbuf_grow(sb, LARGE_PACKET_MAX);   // allocate space 
>> for at least one packet
>> +else
>> +strbuf_grow(sb, st_add(expected_bytes, 1));  // add one extra 
>> byte for the packet flush
>> +
>> +do {
>> +bytes_read = packet_read(
>> +fd_in, NULL, NULL,
>> +sb->buf + total_bytes_read, sb->len - total_bytes_read 
>> - 1,
>> +PACKET_READ_GENTLE_ON_EOF
>> +);
>> +if (bytes_read < 0)
>> +return 1;  // unexpected EOF
> 
> Don't we usually return negative numbers on error?  Ah, I see that the
> return is a bool, which allows to use 

Re: [PATCH] reset cached ident date before creating objects

[Jeff King]

On Mon, Aug 01, 2016 at 10:58:47AM -0700, Junio C Hamano wrote:

> diff --git a/commit.c b/commit.c
> index b02f3c4..db24013 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -1543,7 +1543,6 @@ int commit_tree_extended(const char *msg, size_t 
> msg_len,
>   }
>  
>   /* Person/date information */
> - reset_ident_date();
>   if (!author)
>   author = git_author_info(IDENT_STRICT);
>   strbuf_addf(, "author %s\n", author);
> @@ -1564,11 +1563,21 @@ int commit_tree_extended(const char *msg, size_t 
> msg_len,
>   if (encoding_is_utf8 && !verify_utf8())
>   fprintf(stderr, commit_utf8_warn);
>  
> - if (sign_commit && do_sign_commit(, sign_commit))
> - return -1;
> + if (sign_commit && do_sign_commit(, sign_commit)) {
> + result = -1;
> + goto out;
> + }
>  
>   result = write_sha1_file(buffer.buf, buffer.len, commit_type, ret);
> +
> +out:
>   strbuf_release();
> +
> + /*
> +  * Reset the default timestamp for the next call to create tag/commit
> +  * object, so that they get their own fresh timestamp.
> +  */
> + reset_ident_date();
>   return result;
>  }

Before I switched to "reset at the beginning" in my original patch, I
also noticed this issue, but decided the other way: to only reset after
a successful creation.

I think in most cases it wouldn't matter anyway, because the caller will
generally abort as soon as this returns failure anyway. But I wondered
about something like:

  author = prepare_author_info();
  if (commit_tree_extended(..., author, ...) < 0) {
/* oops, we failed. Do a thing and try again. */
possible_fix();
if (commit_tree_extended(..., author, ...) < 0)
die("giving up");
  }

In the second call (but only the second call!) the committer and author
can diverge.

-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 1/2] pager: move pager-specific setup into the build

[Jeff King]

On Mon, Aug 01, 2016 at 08:01:13PM +0200, Duy Nguyen wrote:

> > If you are interested, I suggest you read the thread linked earlier:
> >
> >   https://public-inbox.org/git/52D87A79.6060600%40rawbw.com/T/#u
> >
> > which discusses this and other issues. But basically, I think you cannot
> > really solve this without getting intimate with each pager (which people
> > seemed not to want to do).
> 
> Cooking pager specifics in git does sound bad. But it does not have to
> be that way. What if we delegate the decision whether to color or not
> to a script (e.g. by setting color.ui= "script ")?
> The script has all the info (env variables, uname, user preference...)
> and can make a better decision than 'is stdout a tty?'. It's not about
> out of the box experience, more towards customization (without
> fragmenting .gitconfig files too much).

It sounds like we are solving two separate problems.

I was mostly concerned with the out-of-the-box experience. E.g., you
build git and run "git log" and it prints out gibberish, either because
of your $PAGER or your $LESS settings (which you might have set years
ago).

For more advanced usage like yours, I'd shove any personal logic into a
wrapper around your pager script.  I think the particular decision you
want, though, is related to color.pager, which is outside that scope.
I'm not sure exactly what your setup looks like, but I wonder if it
would be served by better config support (i.e., why do you need a script
to dynamically look at your pager and see if color.pager should be
turned on; can't you configure both at the same time?).

-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 1/2] format-patch: Add a config option format.from to set the default for --from

[Jeff King]

On Sat, Jul 30, 2016 at 12:11:11PM -0700, Josh Triplett wrote:

> +enum from {
> + FROM_AUTHOR,
> + FROM_USER,
> + FROM_VALUE,
> +};
> +
> +static void set_from(enum from type, const char *value)
> +{
> + free(from);
> + switch (type) {
> + case FROM_AUTHOR:
> + from = NULL;
> + break;
> + case FROM_USER:
> + from = xstrdup(git_committer_info(IDENT_NO_DATE));
> + break;
> + case FROM_VALUE:
> + from = xstrdup(value);
> + break;
> + }
> +}

Thanks for looking into reducing the duplication. TBH, I am not sure it
is really an improvement, just because of the amount of boilerplate (and
this function interface is kind of weird, because of the rules for when
"value" should or should not be NULL).

I guess another way to do it would be:

  #define FROM_AUTO_IDENT ((const char *)(intptr_t)1))
  void set_from(const char *value)
  {
if (value == FROM_AUTO_IDENT)
value = git_committer_info(IDENT_NO_DATE);
free(from);
from = xstrdup_or_null(value);
  }

but I think the effort to polish further here is outweighing the
magnitude of the patch itself. So I offer that as "how I would have done
it" in case you like it, but again, I am fine with either this version
or the previous.

-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 1/2] pager: move pager-specific setup into the build

[Jeff King]

On Mon, Aug 01, 2016 at 07:46:34PM +0200, Duy Nguyen wrote:

> On Mon, Aug 1, 2016 at 3:05 AM, Eric Wong  wrote:
> > From: Junio C Hamano 
> >
> > Allowing PAGER_ENV to be set at build-time allows us to move
> > pager-specific knowledge out of our build.  Currently, this
> > allows us to set a better default for FreeBSD where more(1)
> > is the same binary as less(1).
> 
> Nice. I was just too lazy to do something like this and "export
> PAGER=less LESS=FRX" then ignored it :-P
> 
> Slightly off topic, but pagers like 'more' does not understand colors
> either. But color.ui = auto does not know what and prints color code
> anyway. It would be nice if we had some configuration to describe
> "this pager can show colors, that pager does not" so I don't have to
> maintain separate .gitconfig files on two platforms.

If you are interested, I suggest you read the thread linked earlier:

  https://public-inbox.org/git/52D87A79.6060600%40rawbw.com/T/#u

which discusses this and other issues. But basically, I think you cannot
really solve this without getting intimate with each pager (which people
seemed not to want to do).

-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] reset cached ident date before creating objects

[Junio C Hamano]

Junio C Hamano  writes:

> Jeff King  writes:
>
>>> So maybe we would have to put reset_ident_date() at the end of the
>>> function instead, at least after git_committer_info() is called.
>>
>> Yes, although "reset and end" still feels a bit weird to me.
>>
>> I'd almost prefer to just have long-running programs insert resets at
>> strategic points.
>
> Certainly "reset at the end" feels weird but it can be explained as
> "for a one-shot thing we use the first time of the default date and
> it gives a consistent timestamp; conceptually, things that make
> multiple commits are like doing that one-shot thing multiple times
> in a row."
>
> When viewed that way, it is not _too_ bad, I would guess.

An interdiff to what we queued previously would look like this:

 builtin/tag.c | 11 ++-
 commit.c  | 15 ---
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 5dccae3..e852ded 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -225,7 +225,6 @@ static void create_tag(const unsigned char *object, const 
char *tag,
if (type <= OBJ_NONE)
die(_("bad object type."));
 
-   reset_ident_date();
header_len = snprintf(header_buf, sizeof(header_buf),
  "object %s\n"
  "type %s\n"
@@ -287,6 +286,16 @@ static void create_tag(const unsigned char *object, const 
char *tag,
unlink_or_warn(path);
free(path);
}
+
+   /*
+* Reset the default timestamp for the next call to create tag/commit
+* object, so that they get their own fresh timestamp.
+*
+* NOTE NOTE NOTE! if you are libifying this function later by
+* turning exit/die in the above code to return an error, be
+* sure to jump here to make this call happen.
+*/
+   reset_ident_date();
 }
 
 struct msg_arg {
diff --git a/commit.c b/commit.c
index b02f3c4..db24013 100644
--- a/commit.c
+++ b/commit.c
@@ -1543,7 +1543,6 @@ int commit_tree_extended(const char *msg, size_t msg_len,
}
 
/* Person/date information */
-   reset_ident_date();
if (!author)
author = git_author_info(IDENT_STRICT);
strbuf_addf(, "author %s\n", author);
@@ -1564,11 +1563,21 @@ int commit_tree_extended(const char *msg, size_t 
msg_len,
if (encoding_is_utf8 && !verify_utf8())
fprintf(stderr, commit_utf8_warn);
 
-   if (sign_commit && do_sign_commit(, sign_commit))
-   return -1;
+   if (sign_commit && do_sign_commit(, sign_commit)) {
+   result = -1;
+   goto out;
+   }
 
result = write_sha1_file(buffer.buf, buffer.len, commit_type, ret);
+
+out:
strbuf_release();
+
+   /*
+* Reset the default timestamp for the next call to create tag/commit
+* object, so that they get their own fresh timestamp.
+*/
+   reset_ident_date();
return result;
 }
 
--
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] pager: implement core.pagerEnv in config

[Jeff King]

On Mon, Aug 01, 2016 at 01:05:57AM +, Eric Wong wrote:

> This allows overriding the build-time PAGER_ENV variable
> at run-time.
> 
> Inspired by part 1 of an idea from Kyle J. McKay at:
> https://public-inbox.org/git/62db6def-8b39-4481-ba06-245bf4523...@gmail.com/

This commit message is missing the "why" (I tried to get it from the
referenced email, but I am still confused).

What does this buy you over:

  GIT_PAGER='less -whatever-options-you-like'

? Sure, you have to say "less" there and not just "if we happen to be
using less, use these options with it". But that distinction is
important for a build-time default, not for a run-time one. And by
pointing people to GIT_PAGER, they can do a lot _more_ than they can
with PAGER_ENV, including the full power of the shell (brian gave an
example of "par | less" earlier; I use "diff-highlight | less").

-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 1/2] pager: move pager-specific setup into the build

[Duy Nguyen]

On Mon, Aug 1, 2016 at 3:05 AM, Eric Wong  wrote:
> From: Junio C Hamano 
>
> Allowing PAGER_ENV to be set at build-time allows us to move
> pager-specific knowledge out of our build.  Currently, this
> allows us to set a better default for FreeBSD where more(1)
> is the same binary as less(1).

Nice. I was just too lazy to do something like this and "export
PAGER=less LESS=FRX" then ignored it :-P

Slightly off topic, but pagers like 'more' does not understand colors
either. But color.ui = auto does not know what and prints color code
anyway. It would be nice if we had some configuration to describe
"this pager can show colors, that pager does not" so I don't have to
maintain separate .gitconfig files on two platforms.
-- 
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 v2 0/2] format-patch: Transition the default to --from to avoid spoofed mails

[Jeff King]

On Sat, Jul 30, 2016 at 12:11:05PM -0700, Josh Triplett wrote:

> Josh Triplett (2):
>   format-patch: Add a config option format.from to set the default for --from
>   format-patch: Default to --from

By the way, I notice that the threading between your patches and cover
letter are broken. Since I see you are also working on a tool for
handling such things, I'd suspect the tool (or your workflow) has a bug.
:)

The message-id of this message is:

  <20160730191104.2ps5k7eji7aqgufg@x>

but the patches have both "References" and "In-Reply-To" set to:

  


I also see your MUA is mutt, and I think I saw you mention using "mutt
-H" elsewhere. IIRC, when I started using a similar workflow years ago,
I tried the same thing and had the same problem: "-H" treats the input
file as a template, not a message, and thus generates a new message-id.

I switched to using mutt's internal "resend-message" function, which
does a more literal re-send. I don't think I ever found a way to
convince mutt to do a resend from the command line.

-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] reset cached ident date before creating objects

[Junio C Hamano]

Jeff King  writes:

>> So maybe we would have to put reset_ident_date() at the end of the
>> function instead, at least after git_committer_info() is called.
>
> Yes, although "reset and end" still feels a bit weird to me.
>
> I'd almost prefer to just have long-running programs insert resets at
> strategic points.

Certainly "reset at the end" feels weird but it can be explained as
"for a one-shot thing we use the first time of the default date and
it gives a consistent timestamp; conceptually, things that make
multiple commits are like doing that one-shot thing multiple times
in a row."

When viewed that way, it is not _too_ bad, I would guess.

--
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] git-format-patch: default to --from to avoid spoofed mails?

[Jeff King]

On Mon, Aug 01, 2016 at 10:35:22AM -0700, Junio C Hamano wrote:

> > I do have to admit that after reading through the "format.*" section of
> > git-config(1), there is quite a bit that is configurable in it. So
> > perhaps we do not need to be as careful about behavior changes as I
> > thought.
> 
> I am not sure how the first sentence (which I agree with; a random
> user can have quite a different behaviour configured when the
> command is run without any option) leads to the conclusion in the
> second sentence.  The user can break assumptions made by a tool that
> reads format-patch output by tweaking his config but at least he
> knows that he changed the configuration, i.e. the breakage can be
> explained and attributed to his own action.  The change in the
> default is somewhat different.

I half-agree. Config that causes unpredictable behavior can break
somebody else's script that you are running. If you say "oh, I guess I
shouldn't set that config" and move on with your life, then the config
hasn't really hurt anybody. If you complain to the script author that
their script is broken, and insist that they pass the --no-foo option,
then the script writer does not care much whether it was a config option
or a change of default.

-Peff

PS We also changed the default pager behavior for format-patch recently,
   though I can't actually think of any script regressions that would
   cause, since it only kicks in when output is going to the tty.
--
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] git-format-patch: default to --from to avoid spoofed mails?

[Junio C Hamano]

Jeff King  writes:

> On Fri, Jul 29, 2016 at 09:50:55PM -0700, Josh Triplett wrote:
>
>> I would propose the following then:
>> 
>> - I'll write a patch adding a config option format.from, along with
>>   documentation, without changing the default.
>> - The release notes for the version of git introducing that config
>>   option should mention, prominently, the plan to change the default in
>>   a future version of git.
>> - A subsequent release can change the default.  No major rush to do
>>   this.
>> 
>> Does that sound reasonable?
>
> That sounds fine to me.

To me, too.

> I do have to admit that after reading through the "format.*" section of
> git-config(1), there is quite a bit that is configurable in it. So
> perhaps we do not need to be as careful about behavior changes as I
> thought.

I am not sure how the first sentence (which I agree with; a random
user can have quite a different behaviour configured when the
command is run without any option) leads to the conclusion in the
second sentence.  The user can break assumptions made by a tool that
reads format-patch output by tweaking his config but at least he
knows that he changed the configuration, i.e. the breakage can be
explained and attributed to his own action.  The change in the
default is somewhat different.

When we _know_ we are going to be changing the default, we should
forewarn in previous releases (in release notes, and perhaps we
would want to have a runtime warning when the user formats others'
changes without having format.from explicitly set to either true or
false).

So the second step can be delayed and does not have to be done for
the release that includes the first change.  But I am not sure how
"there are many format.* configuration" leads to "we just announce
that we changed the default and tell peole there is a new knob to
retain the original behaviour".

> So if you wanted to squish steps 2 and 3 together, that would also be OK
> by me.
--
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] format-patch: Add a config option format.from to set the default for --from

[Jeff King]

On Sat, Jul 30, 2016 at 11:12:46AM -0700, Josh Triplett wrote:

> > These tests follow a different style from the "--from" tests later in
> > the script (and your second patch does follow it, and puts its test
> > close there). Any reason not to have all of the "from" tests together,
> > and using the same style?
> 
> The tests covered different things.  The later --from tests made sure
> that --from behaved as expected.  These tests made sure that format.from
> and --from/--no-from interacted in the expected way, with the
> command-line options overriding the configuration.  So, I put them next
> to the tests for other options like format.to and format.cc, which
> tested the same thing (overriding those with --no-to, --no-cc, etc).

OK. I would have grouped by "things that influence this area of
behavior", not by "config versus command-line". But I don't think either
is wrong or right. And since you are the one writing the patch, "how I
would have done it" is not a compelling review comment.

-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 1/2] pager: move pager-specific setup into the build

[Jeff King]

On Mon, Aug 01, 2016 at 01:43:03AM +, brian m. carlson wrote:

> On Mon, Aug 01, 2016 at 01:05:56AM +, Eric Wong wrote:
> > +static void setup_pager_env(struct argv_array *env)
> > +{
> > +   const char *pager_env = stringify(PAGER_ENV);
> > +
> > +   while (*pager_env) {
> > +   struct strbuf buf = STRBUF_INIT;
> > +   const char *cp = strchrnul(pager_env, '=');
> > +
> > +   if (!*cp)
> > +   die("malformed build-time PAGER_ENV");
> > +   strbuf_add(, pager_env, cp - pager_env);
> > +   cp = strchrnul(pager_env, ' ');
> > +   if (!getenv(buf.buf)) {
> > +   strbuf_reset();
> > +   strbuf_add(, pager_env, cp - pager_env);
> > +   argv_array_push(env, strbuf_detach(, NULL));
> > +   }
> > +   strbuf_reset();
> > +   while (*cp && *cp == ' ')
> > +   cp++;
> > +   pager_env = cp;
> > +   }
> 
> So it looks like this function splits on spaces but doesn't provide any
> escaping mechanism.  Is there any case in which we want to accept
> environment variables containing whitespace?  I ask this as someone that
> has EDITOR set to "gvim -f" on occasion and seeing how tools sometimes
> handle that poorly.
> 
> Even without that, I think this series is probably an improvement over
> the status quo.

I'm not too worried about spaces here. This is a resurrection of an old
discussion, and in all that time, I think the only realistic suggestions
for built-in values have been pretty tame.

If this were used to parse arbitrary user-provided runtime values, I'd
be more concerned. But I'm not sure why we would need that. Your $EDITOR
example is arbitrary shell code, and we let the shell handle it (modulo
some efficiency shortcuts). Likewise, fancy runtime things should go in
GIT_PAGER, where you can not only set options with spaces, but do fancy
things like pipes, shell functions, etc.

The use of stringify() here is funny to me; I think there is a cpp
tokenizing step in the middle that will do things like gobble up
whitespace (but I'm not sure if it has other possible effects). I think
our more usual method here would be to C-quote in the Makefile (with the
equivalent of 's/\\//g; s/"/\\"/g'), and then pass it to the
compiler as a string literal, like -DPAGER_ENV=\"$(PAGER_ENV_CQ_SQ\".

-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 bisect for reachable commits only

[Christian Couder]

On Mon, Aug 1, 2016 at 12:02 PM, Oleg Taranenko  wrote:
> Guys,
>
> further investigation shows, git bisect is broken from its core... really.
> Let consider 3rd a bit more complicated scenario
>
> #cd ..
> #rm -rf bisect3
> mkdir bisect3
> cd bisect3
> git init
> git touch coffee

git touch is not a git command

> git commit -am "create coffee"

you need to "git add coffee" first otherwise it doesn't work

> git branch tee
> echo sugar >> coffee
> git commit -am "add sugar"  # we are still in master branch
> echo "milk" >> coffee
> git commit -am "milk for coffee"
> ex +g/sugar/d -cwq coffee  # introducing 'bug'
> git commit -am "somehow remove sugar"
> echo "mixing..." >> coffee
> git commit -am "coffee mixing"
>
> git checkout tee# get back to coffee without sugar
> git touch tee

git touch is not a git command
it might be an alias you have that does `touch "$@" && git add "$@"`

> git commit -am "tee"
>
> git branch cocktail
> echo "sugar" >> tee
> git commit -am "sugar for tee"
> echo "milk" >> tee
> git commit -am "milk for tee"
> echo "mixing..." >> tee
> git commit -am "tee mixing"
>
> git checkout cocktail
> git touch cocktail
> git commit -am "prepare cocktail"
> echo orange >>cocktail
> git commit -am "add orange juice"
> echo rum >>cocktail
> git commit -am "add rum"
> echo mixing >> cocktail
> git commit -am "cocktail mixing"
> cat cocktail  #orange, rum, mixing
> git merge tee
> git merge master
>
> git touch serve
> git commit -am "serving..."
>
> git log --full-history --graph --pretty=oneline
>
> * 059adf903a2cbc06fe05dda4c916e2c586907f23 serving...
> *   efc89d5253d3126defc7362c25ef069ae9b43fc7 Merge branch 'master' into 
> cocktail
> |\
> | * dd41e230a3cac5d51a1e994747ff470e2af03cae coffee mixing
> | * c2a44672f1197f34e04cd0fd66434a2b286b574e somehow remove sugar
> | * f50352cfb6bc4a237b73c95ed7ebca074603ae11 milk for coffee
> | * 79b253b316cdc3668697afe473610e35b453ab2f add sugar
> * |   2d626eb5cfaa40a4503be58a5ed27f1ececa6d02 Merge branch 'tee' into 
> cocktail
> |\ \
> | * | 7aba690c6c6f73f1906871c9dbf9737ec11a152b tee mixing
> | * | eca611a93697359ec7a52f4a045461180bc365c3 milk for tee
> | * | 7d6844724d0e81751ec1a67c1ffdf0d0fb932350 sugar for tee
> * | | 6754e816922989d5870ec3452437bbbe6aca4d0f cocktail mixing
> * | | 5cbbf0f0882c497590838b163210db3a393b647e add rum
> * | | b46d7d8a361daae382fbef7acabda5416d23da46 add orange juice
> * | | e571fdd09582e40fc54ffc5a4f112eac2b9f2c8e prepare cocktail
> |/ /
> * | 041a5a53704bccc60c489f8c9a4742bad79d5a95 tee
> |/
> * a52a4fa6770d000a9f4e9e297739a6dc88c0cc50 create coffee
>
> As you can see, no tricks with amended history, but...
>
> git bisect start HEAD 79b2
> Bisecting: 8 revisions left to test after this (roughly 3 steps)
> [6754e816922989d5870ec3452437bbbe6aca4d0f] cocktail mixing
> cat coffee
> git bisect bad

Why is it bad?
Is it because there is no sugar?
In this case you are searching for a commit that removed sugar.

> Bisecting: 2 revisions left to test after this (roughly 1 step)
> [e571fdd09582e40fc54ffc5a4f112eac2b9f2c8e] prepare cocktail
> git bisect bad
> Bisecting: 0 revisions left to test after this (roughly 0 steps)
> [041a5a53704bccc60c489f8c9a4742bad79d5a95] tee
> git bisect bad
> 041a5a53704bccc60c489f8c9a4742bad79d5a95 is the first bad commit
> commit 041a5a53704bccc60c489f8c9a4742bad79d5a95
> Author: Oleg Taranenko 
> Date:   Mon Aug 1 10:53:52 2016 +0200
>
> tee
>
> :00 100644 
> e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 A tee
>
> git bisect ever not looked into the path where good commit is
> declared. Instead it found somehow most common ancestor from whole
> tree (a52a create coffee),  assume it is GOOD commit (why !?)

Yes, and the reason is that all the ancestors of a good commit are
considered good.
That's because git bisect supposes that there is only one transition
from good to bad.
Otherwise we would need a more complex algorithm to find all the
transitions from good to bad, and that is not generally needed.

> and
> check only ^1 (not ^2) parent commit for testing as a potential bug
> commit.
> No wonder now, I got a disaster result, looking in my heavy enterprise
> repository.
>
> Can somebody take care of this issue?

I haven't looked at your previous issue much, sorry I have been busy these days.
--
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] diff-highlight: add support for --graph output.

[Junio C Hamano]

Brian Henderson  writes:

> ---
>  contrib/diff-highlight/diff-highlight | 13 +++--
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/contrib/diff-highlight/diff-highlight 
> b/contrib/diff-highlight/diff-highlight
> index ffefc31..ec31356 100755
> --- a/contrib/diff-highlight/diff-highlight
> +++ b/contrib/diff-highlight/diff-highlight
> @@ -20,6 +20,7 @@ my @NEW_HIGHLIGHT = (
>  my $RESET = "\x1b[m";
>  my $COLOR = qr/\x1b\[[0-9;]*m/;
>  my $BORING = qr/$COLOR|\s/;
> +my $GRAPH = qr/$COLOR?\|$COLOR?\s+/;

I didn't read the other parts (or existing code this patch changes,
for that matter) of the series, but this looks like an attempt to
catch the leading "|" bar showing direct ancestry.  It makes a
reader wonder what happens in a mergy histroy, though.

I _think_ that the patch portion of "log -p" output would only have
"|" and never "\" or "/" that are used to adjust the number of
tracks to deal with forks and merges, but perhaps the fact that this
code relies on that assumption deserves to be written down here as
an in-code comment?

>  my @removed;
>  my @added;
> @@ -32,12 +33,12 @@ $SIG{PIPE} = 'DEFAULT';
>  while (<>) {
>   if (!$in_hunk) {
>   print;
> - $in_hunk = /^$COLOR*\@/;
> + $in_hunk = /^$GRAPH*$COLOR*\@/;
>   }
> - elsif (/^$COLOR*-/) {
> + elsif (/^$GRAPH*$COLOR*-/) {
>   push @removed, $_;
>   }
> - elsif (/^$COLOR*\+/) {
> + elsif (/^$GRAPH*$COLOR*\+/) {
>   push @added, $_;
>   }
>   else {
> @@ -46,7 +47,7 @@ while (<>) {
>   @added = ();
>  
>   print;
> - $in_hunk = /^$COLOR*[\@ ]/;
> + $in_hunk = /^$GRAPH*$COLOR*[\@ ]/;
>   }
>  
>   # Most of the time there is enough output to keep things streaming,
> @@ -211,8 +212,8 @@ sub is_pair_interesting {
>   my $suffix_a = join('', @$a[($sa+1)..$#$a]);
>   my $suffix_b = join('', @$b[($sb+1)..$#$b]);
>  
> - return $prefix_a !~ /^$COLOR*-$BORING*$/ ||
> -$prefix_b !~ /^$COLOR*\+$BORING*$/ ||
> + return $prefix_a !~ /^$GRAPH*$COLOR*-$BORING*$/ ||
> +$prefix_b !~ /^$GRAPH*$COLOR*\+$BORING*$/ ||
>  $suffix_a !~ /^$BORING*$/ ||
>  $suffix_b !~ /^$BORING*$/;
>  }
--
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] use strbuf_addstr() for adding constant strings to a strbuf

[Jeff King]

On Sat, Jul 30, 2016 at 07:36:23PM +0200, René Scharfe wrote:

> Replace uses of strbuf_addf() for adding strings with more lightweight
> strbuf_addstr() calls.
> 
> In http-push.c it becomes easier to see what's going on without having
> to verfiy that the definition of PROPFIND_ALL_REQUEST doesn't contain
> any format specifiers.

Nice. I care a lot less about whether "addf" or "addstr" is more
efficient. But the second point, that it makes the intent clearer, is a
big win.

-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] t7063: work around FreeBSD's lazy mtime update feature

[Duy Nguyen]

On Mon, Aug 1, 2016 at 3:37 AM, Torstem Bögershausen  wrote:
> the term FREEBSD may be too generic to point out a single feature
> in an OS distributution.
> Following your investigations, it may even be possible that
> other systems adapt this "feature"?
>
> How about
> LAZY_DIR_MTIME_UPDATE
> (or similar)

This feature was added in 1998, so yes there's a chance it has spread
to a few fbsd derivatives (not sure if openbsd or netbsd is close
enough and they ever exchange changes). But I'd rather wait for the
second OS to expose the same feature before renaming it.
-- 
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: git bisect for reachable commits only

[Junio C Hamano]

Jakub Narębski  writes:

> Isn't `--reachable-commits` the same as existing `--ancestry-path`
> option to `git log` and friends (I wonder if passing log options to
> bisect, that is: `git bisect --ancestry-path ...` would work)?

Yes, I somehow missed it, but you are absolutely correct to point
out that what is being requested is --ancestry-path.

My gut feeling is that by default the command should be taught to
follow the ancestry path, but I say this with reservation, as I am
not sure offhand what it means to traverse along the ancestry path
when there are multiple good commits.

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 v9 39/41] apply: change error_routine when silent

[Stefan Beller]

On Sat, Jul 30, 2016 at 10:25 AM, Christian Couder
 wrote:
> +   /* This is to save some reporting routines */

some?

In case of a reroll could you be more specific?
Specifically mention that we use these for the
muting/when silence is requested.
e.g.
/* These control reporting routines, and are used e.g. for muting output */

> +   void (*saved_error_routine)(const char *err, va_list params);
> +   void (*saved_warn_routine)(const char *warn, va_list params);
> +
--
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] pass constants as first argument to st_mult()

[Jeff King]

On Sat, Jul 30, 2016 at 08:18:31PM +0200, René Scharfe wrote:

> The result of st_mult() is the same no matter the order of its
> arguments.  It invokes the macro unsigned_mult_overflows(), which
> divides the second parameter by the first one.  Pass constants
> first to allow that division to be done already at compile time.

I'm not opposed to this, as it's easy to do (though I suspect new calls
may be introduced that violate it).

But if we really are worried about the performance of st_mult(), I
think:

  static inline size_t st_mult(size_t a, size_t b)
  {
size_t result;
if (!__builtin_mul_overflow(a, b, ))
die("whoops!");
return result;
  }

is the right direction. I just haven't gotten around to producing a
polished patch.

> diff --git a/diffcore-rename.c b/diffcore-rename.c
> index 58ac0a5..73d003a 100644
> --- a/diffcore-rename.c
> +++ b/diffcore-rename.c
> @@ -541,7 +541,7 @@ void diffcore_rename(struct diff_options *options)
>   rename_dst_nr * rename_src_nr, 50, 1);
>   }
>  
> - mx = xcalloc(st_mult(num_create, NUM_CANDIDATE_PER_DST), sizeof(*mx));
> + mx = xcalloc(st_mult(NUM_CANDIDATE_PER_DST, num_create), sizeof(*mx));

I didn't look at all of the calls, but I wonder if it is a natural
pattern to put the constant second.  Since multiplication is
commutative, it would be correct for st_mult() to just flip the order of
arguments it feeds to unsigned_mult_overflows().

That may introduce the same inefficiency in other callsites, but I
wonder if it would be fewer.

-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 v9 03/41] builtin/apply: make apply_patch() return -1 or -128 instead of die()ing

[Stefan Beller]

On Sat, Jul 30, 2016 at 10:24 AM, Christian Couder
 wrote:

> + * Try to apply a patch.
> + *
> + * Returns:
> + *  -128 if a bad error happened (like patch unreadable)
> + *  -1 if patch did not apply and user cannot deal with it
> + *   0 if the patch applied
> + *   1 if the patch did not apply but user might fix it

I stopped and wondered when reading this comment,
what the difference between -1 and 1 is, as the user is
not part of this function.

When reading the code, this makes sense, though.
So -1 is returned when the user set `apply_with_reject`,
1 otherwise? So the user told us upfront how to deal with
certain errors.

What is a "bad" error, that generates a -128?
(Only when the patch is not syntactically correct? Or are there
other -128 errors as well?)

Maybe:
 Returns zero on success,
 non zero for failing to apply a patch
 negative values for hard errors, e.g. unreadable patc.

Though this is less precise, as it doesn't differentiate between
-1 and -128. I dunno.

(These are just musings that should not stop going
forward with this patch, just some thoughts on the precision
of a comment)

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 v9 04/41] builtin/apply: read_patch_file() return -1 instead of die()ing

[Stefan Beller]

On Sat, Jul 30, 2016 at 10:24 AM, Christian Couder
 wrote:

> -static void read_patch_file(struct strbuf *sb, int fd)
> +static int read_patch_file(struct strbuf *sb, int fd)
>  {
> if (strbuf_read(sb, fd, 0) < 0)
> -   die_errno("git apply: failed to read");
> +   return error_errno("git apply: failed to read");

which always returns -1.

> @@ -4425,7 +4426,8 @@ static int apply_patch(struct apply_state *state,
> int res = 0;
>
> state->patch_input_file = filename;
> -   read_patch_file(, fd);
> +   if (read_patch_file(, fd))

In case a reroll turns out to be needed, check for
"read_patch_file(..) < 0" here,
as we only want to error out in case of errors from that function?
The return value of read_patch_file, is not documented as it seems
trivial at the
moment, i.e.

  0 for success
  negative values for errors
  positive values are currently not returned, but are reserved for future use?

The current implementation is correct as-is, though I think we follow the
"negative values indicate a serious error and positive values are to
be expected,
and not necessarily an error" pattern in lots of other places, so we
could here as well.
--
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 7/8] status: update git-status.txt for --porcelain=v2

[Jeff Hostetler]

On 07/30/2016 01:22 PM, Jakub Narębski wrote:

W dniu 26.07.2016 o 23:11, Jeff Hostetler pisze:

This is a nice change, available because of lack of backward
compatibility with v1.  The porcelain v2 format branch-related
information could be enhanced without risk of breaking parsers,
or having new information put at the end even if it does not fit
there (like in previous iteration).

One thing that can serve as goal for the series is using the
question: would it make __git_ps1() from git-prompt.sh be able
to render fully decorated prompt with all bells and whistles,
and with all combinations of options.  Thus beside upstream
in the future we might want SVN upstream, and/or pushed-to
remote branch (and remote push upstream repository), etc.

But that's for the future (and it is possible for the future)...



Yes, I was hoping to be able to simplify and/or speed up
__git_ps1() with this data.  "Namespacing" the branch data
here.  And then later add the state data (in a merge,
in a rebase, and etc.) in a series of "# state.*" headers.
And so on, until we get everything that __git_ps1() needs.
However, to really make that work, we might want to add
a --no-scan (or minimial scan) option, to just return the
header data, since __git_ps1() doesn't care about the list
of changes.



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


Nice solution to avoid those all zeros / null-SHA1s


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 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 question: do unmerged entries always have only one single filename?
Or unmerged entries are always only about CONFLICT(contents), and no
other?


As far as I could tell, unmerged rename conflicts appear with a single
filename.

When I did a divergent rename (in both branches), merge creates
a stage-1 "DD" entry,
a stage-2 "AU" entry, and
a stage-3 with a "UA" entry.
Status reports it as 3 rows.

When I did a rename in one branch and an edits in both, merge
creates: either a "DU" or "UD" conflict (depending on the direction
of the merge).

Given that the index is ordered and accessed by path (and there is
no pathname field in a cache entry to link it to a different one),
I have to say that this is true.



Would a note (or a link to other documentation) about octopus merges
be out of place here?


I put a note in the code about it reporting the last entry at each
stage that are present in the index, but I'm not sure about how much
we want to say here.

If octopus finds conflicts, the worktree will probably be in a
funky state anyway.




+
+A series of lines are then displayed for untracked and ignored entries.
+
+ 
+
+Where  is "?" for untracked entries and "!" for ignored entries.


A question: are they displayed in that order, i.e. first all untracked,
then all ignored, or it is something one cannot rely about?


With the unique prefix character it shouldn't matter.  I do print
all the '?' lines first then all the '!' lines, so the manpage
could be clarified if we wanted. I was just trying to save another
paragraph in the manpage.




+
+In all 3 line formats, pathnames will be "C Quoted" if they contain


"C Quoted" or "C-Quoted"?  How it is described in other places of
the Git documentation?



I was probably inconsistent, I think it should be "c-quoted" (with the
hypen) providing we like this term.

Each of the git-diff*.txt and diff-format.txt files talk about
this quoting scheme, but none give it an explicit name.



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


Does it mean that

 2 [...] \t\n

line (including line terminator) is replaced with

 2 [...] \0\0

that is, it replaces a separator (TAB, "\t") and line 

Re: [ANNOUNCE] git-series: track changes to a patch series over time

[Stephen Warren]

On 07/29/2016 12:40 AM, Josh Triplett wrote:

I'd like to announce a project I've been working on for a while:

git-series provides a tool for managing patch series with git, tracking
the "history of history". git series tracks changes to the patch series
over time, including rebases and other non-fast-forwarding changes. git
series also tracks a cover letter for the patch series, formats the
series for email, and prepares pull requests.


Just as an FYI, I wouldn't be surprised if there's some overlap, or 
potential for merging of tools, between this tool and the "patman" tool 
that's part of the U-Boot source tree:


http://git.denx.de/?p=u-boot.git;a=blob;f=tools/patman/README;h=e36857dedea1d0dbafa41732aaf9bf0988d63f38;hb=HEAD

--
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 10/10] convert: add filter..process option

[Lars Schneider]

> On 31 Jul 2016, at 00:05, Jakub Narębski  wrote:
> 
> W dniu 30.07.2016 o 01:38, larsxschnei...@gmail.com pisze:
>> 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.
> 
> I think it would be nice to have here at least summary of the benchmarks
> you did in https://github.com/github/git-lfs/pull/1382

OK.


>> Git starts the filter on first usage and expects a welcome
>> message, protocol version number, and filter capabilities
>> separated by spaces:
>> 
>> packet:  git< git-filter-protocol\n
>> packet:  git< version 2\n
>> packet:  git< capabilities clean smudge\n
> 
> Sorry for going back and forth, but now I think that 'capabilities' are
> not really needed here, though they are in line with "version" in
> the second packet / line, namely "version 2".  If it does not make
> parsing more difficult...

I don't understand what you mean with "they are not really needed"?
The field is necessary to understand the protocol, no?

In the last roll I added the "key=value" format to the protocol upon
yours and Peff's suggestion. Would it be OK to change the startup
sequence accordingly?

packet:  git< version=2\n
packet:  git< capabilities=clean smudge\n


>> 
>> Supported filter capabilities are "clean", "smudge", "stream",
>> and "shutdown".
> 
> I'd rather put "stream" and "shutdown" capabilities into separate
> patches, for easier review.

I agree with "shutdown". I think I would like to remove the "stream"
option and make it the default for the following reasons:

(1) As you mentioned elsewhere, "stream" is not really streaming at this
point because we don't read/write in parallel.

(2) Junio and you pointed out that if we transmit size and flush packet
then we have redundancy in the protocol.

(3) With the newly introduced "success"/"reject"/"failure" packet at the 
end of a filter operation, a filter process has a way to signal Git that
something went wrong. Initially I had the idea that a filter process just
stops writing and Git would detect the mismatch between expected bytes
and received bytes. But the final status packet is a much clearer solution.

(4) Maintaining two slightly different protocols is a waste of resources 
and only increases the size of this (already large) patch.

My only argument for the size packet was that this allows efficient buffer
allocation. However, in non of my benchmarks this was actually a problem.
Therefore this is probably a epsilon optimization and should be removed.

OK with everyone?


>> Afterwards Git sends a command (based on the supported
>> capabilities), the filename including its path
>> relative to the repository root, the content size as ASCII number
>> in bytes, the content split in zero or many pkt-line packets,
>> and a flush packet at the end:
> 
> I guess the following is the most basic example, with mode detailed
> description left for the documentation.
> 
>> 
>> packet:  git> smudge\n
>> packet:  git> filename=path/testfile.dat\n
>> packet:  git> size=7\n
> 
> So I see you went with "=" idea, rather than ""
> (with  defined by position in a sequence of 'header' packets),
> or " ..." that introductory header uses.

The implementation still requires the exact sequence of the packets.
However, we could make this more flexible in a later patch with the
"key=value" formatting.

> 
>> packet:  git> CONTENT
>> packet:  git> 
>> 
>> 
>> The filter is expected to respond with the result content size as
>> ASCII number in bytes. If the capability "stream" is defined then
>> the filter must not send the content size. Afterwards the result
>> content in send in zero or many pkt-line packets and a flush packet
>> at the end. 
> 
> If it does not cost filter anything, it could send size upfront
> (based on size of original, or based on external data), even if
> it is prepared for streaming.
> 
> In the opposite case, where filter cannot stream because it requires
> whole contents upfront (e.g. to calculate hash of the contents, or
> to do operation that needs whole file like sorting or reversing lines),
> it should always be able to calculate the size... or not.  For
> example 'sort | uniq' filter needs whole input upfront for sort,
> but it does not know how many lines will be in output without doing
> the 'uniq' part.
> 
> So I think the ability of 

Re: [PATCH v3 03/10] pkt-line: add packet_flush_gentle()

[Lars Schneider]

> On 30 Jul 2016, at 14:04, Jakub Narębski  wrote:
> 
> W dniu 30.07.2016 o 01:37, larsxschnei...@gmail.com pisze:
>> From: Lars Schneider 
>> 
>> packet_flush() would die in case of a write error even though for some 
>> callers
>> an error would be acceptable. Add packet_flush_gentle() which writes a 
>> pkt-line
>> flush packet and returns `0` for success and `1` for failure.
> 
> I think it should be packet_flush_gently(), as in "to flush gently",
> but this is only my opinion; I have not checked the naming rules and
> practices for the rest of Git codebase.

Agreed. This would match:

object.c:int type_from_string_gently(const char *str, ssize_t len, int gentle)

Thanks,
Lars

> 
>> 
>> Signed-off-by: Lars Schneider 
>> ---
> 

--
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 05/10] pack-protocol: fix maximum pkt-line size

[Lars Schneider]

> On 30 Jul 2016, at 15:58, Jakub Narębski  wrote:
> 
> W dniu 30.07.2016 o 01:37, larsxschnei...@gmail.com pisze:
>> From: Lars Schneider 
>> 
>> According to LARGE_PACKET_MAX in pkt-line.h the maximal lenght of a
>> pkt-line packet is 65520 bytes. The pkt-line header takes 4 bytes and
>> therefore the pkt-line data component must not exceed 65516 bytes.
> 
> s/lenght/length/

Thanks!


> Is it maximum length of pkt-line packet, or maximum length of data
> that can be send in a packet?

65520 is the maximum length of a pkt-line.


> With 4 hex digits, maximal length if pkt-line packet (together
> with length) is _16, that is 2^16-1 = 65535.  Where does the
> number 65520 comes from?

Historic reasons, I guess? However, it won't be changed. See response
from Peff here:
http://public-inbox.org/git/20160726134257.GB19277%40sigill.intra.peff.net/


> 
>> 
>> Signed-off-by: Lars Schneider 
>> ---
>> Documentation/technical/protocol-common.txt | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/Documentation/technical/protocol-common.txt 
>> b/Documentation/technical/protocol-common.txt
>> index bf30167..ecedb34 100644
>> --- a/Documentation/technical/protocol-common.txt
>> +++ b/Documentation/technical/protocol-common.txt
>> @@ -67,9 +67,9 @@ with non-binary data the same whether or not they contain 
>> the trailing
>> LF (stripping the LF if present, and not complaining when it is
>> missing).
>> 
>> -The maximum length of a pkt-line's data component is 65520 bytes.
>> -Implementations MUST NOT send pkt-line whose length exceeds 65524
>> -(65520 bytes of payload + 4 bytes of length data).
>> +The maximum length of a pkt-line's data component is 65516 bytes.
>> +Implementations MUST NOT send pkt-line whose length exceeds 65520
>> +(65516 bytes of payload + 4 bytes of length data).
>> 
>> Implementations SHOULD NOT send an empty pkt-line ("0004").
>> 
>> 
> 

--
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 04/10] pkt-line: call packet_trace() only if a packet is actually send

[Lars Schneider]

> On 30 Jul 2016, at 14:29, Jakub Narębski  wrote:
> 
> W dniu 30.07.2016 o 01:37, larsxschnei...@gmail.com pisze:
>> From: Lars Schneider 
>> 
>> The packet_trace() call is not ideal in format_packet() as we would print
> 
> Style; I think the following is more readable:
> 
>  The packet_trace() call in format_packet() is not ideal, as we would...

Agreed!


>> a trace when a packet is formatted and (potentially) when the packet is
>> actually send. This was no problem up until now because format_packet()
>> was only used by one function. Fix it by moving the trace call into the
>> function that actally sends the packet.
> 
> s/actally/actually/

Thanks!


> I don't buy this explanation.  If you want to trace packets, you might
> do it on input (when formatting packet), or on output (when writing
> packet).  It's when there are more than one formatting function, but
> one writing function, then placing trace call in write function means
> less code duplication; and of course the reverse.
> 
> Another issue is that something may happen between formatting packet
> and sending it, and we probably want to packet_trace() when packet
> is actually send.
> 
> Neither of those is visible in commit message.

The packet_trace() call in format_packet() is not ideal, as we would print
a trace when a packet is formatted and (potentially) when the same packet is
actually written. This was no problem up until now because packet_write(),
the function that uses format_packet() and writes the formatted packet,
did not trace the packet.

This developer believes that trace calls should only happen when a packet
is actually written as the packet could be modified between formatting
and writing. Therefore the trace call was moved from format_packet() to 
packet_write().

--

Better?

> 
>> 
>> Signed-off-by: Lars Schneider 
>> ---
>> pkt-line.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/pkt-line.c b/pkt-line.c
>> index 1728690..32c0a34 100644
>> --- a/pkt-line.c
>> +++ b/pkt-line.c
>> @@ -126,7 +126,6 @@ static void format_packet(struct strbuf *out, const char 
>> *fmt, va_list args)
>>  die("protocol error: impossibly long line");
>> 
>>  set_packet_header(>buf[orig_len], n);
>> -packet_trace(out->buf + orig_len + 4, n - 4, 1);
>> }
>> 
>> void packet_write(int fd, const char *fmt, ...)
>> @@ -138,6 +137,7 @@ void packet_write(int fd, const char *fmt, ...)
>>  va_start(args, fmt);
>>  format_packet(, fmt, args);
>>  va_end(args);
>> +packet_trace(buf.buf + 4, buf.len - 4, 1);
>>  write_or_die(fd, buf.buf, buf.len);
>> }
>> 
>> 
> 

--
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 02/10] pkt-line: add direct_packet_write() and direct_packet_write_data()

[Lars Schneider]

> On 30 Jul 2016, at 12:49, Jakub Narębski  wrote:
> 
> W dniu 30.07.2016 o 01:37, larsxschnei...@gmail.com pisze:
>> From: Lars Schneider 
>> 
>> Sometimes pkt-line data is already available in a buffer and it would
>> be a waste of resources to write the packet using packet_write() which
>> would copy the existing buffer into a strbuf before writing it.
>> 
>> If the caller has control over the buffer creation then the
>> PKTLINE_DATA_START macro can be used to skip the header and write
>> directly into the data section of a pkt-line (PKTLINE_DATA_LEN bytes
>> would be the maximum). direct_packet_write() would take this buffer,
>> adjust the pkt-line header and write it.
>> 
>> If the caller has no control over the buffer creation then
>> direct_packet_write_data() can be used. This function creates a pkt-line
>> header. Afterwards the header and the data buffer are written using two
>> consecutive write calls.
> 
> I don't quite understand what do you mean by "caller has control
> over the buffer creation".  Do you mean that caller either can write
> over the buffer, or cannot overwrite the buffer?  Or do you mean that
> caller either can allocate buffer to hold header, or is getting
> only the data?

How about this:

[...]

If the caller creates the buffer then a proper pkt-line buffer with header
and data section can be created. The PKTLINE_DATA_START macro can be used 
to skip the header section and write directly to the data section 
(PKTLINE_DATA_LEN 
bytes would be the maximum). direct_packet_write() would take this buffer, 
fill the pkt-line header section with the appropriate data length value and 
write the entire buffer.

If the caller does not create the buffer, and consequently cannot leave room
for the pkt-line header, then direct_packet_write_data() can be used. This 
function creates an extra buffer for the pkt-line header and afterwards writes
the header buffer and the data buffer with two consecutive write calls.

---
Is that more clear?

> 
>> 
>> Both functions have a gentle parameter that indicates if Git should die
>> in case of a write error (gentle set to 0) or return with a error (gentle
>> set to 1).
> 
> So they are *_maybe_gently(), isn't it ;-)?  Are there any existing
> functions in Git codebase that take 'gently' / 'strict' / 'die_on_error'
> parameter?

Yes, git grep "gentle" reveals:

wrapper.c:static int memory_limit_check(size_t size, int gentle)
object.c:int type_from_string_gently(const char *str, ssize_t len, int gentle)


>> 
>> Signed-off-by: Lars Schneider 
>> ---
>> pkt-line.c | 30 ++
>> pkt-line.h |  5 +
>> 2 files changed, 35 insertions(+)
>> 
>> diff --git a/pkt-line.c b/pkt-line.c
>> index 445b8e1..6fae508 100644
>> --- a/pkt-line.c
>> +++ b/pkt-line.c
>> @@ -135,6 +135,36 @@ void packet_write(int fd, const char *fmt, ...)
>>  write_or_die(fd, buf.buf, buf.len);
>> }
>> 
>> +int direct_packet_write(int fd, char *buf, size_t size, int gentle)
>> +{
>> +int ret = 0;
>> +packet_trace(buf + 4, size - 4, 1);
>> +set_packet_header(buf, size);
>> +if (gentle)
>> +ret = !write_or_whine_pipe(fd, buf, size, "pkt-line");
>> +else
>> +write_or_die(fd, buf, size);
> 
> Hmmm... in gently case we get the information in the warning that
> it is about "pkt-line", which is missing from !gently case.  But
> it is probably not important.
> 
>> +return ret;
>> +}
> 
> Nice clean function, thanks to extracting set_packet_header().
> 
>> +
>> +int direct_packet_write_data(int fd, const char *buf, size_t size, int 
>> gentle)
> 
> I would name the parameter 'data', rather than 'buf'; IMVHO it
> better describes it.

Agreed!

> 
>> +{
>> +int ret = 0;
>> +char hdr[4];
>> +set_packet_header(hdr, sizeof(hdr) + size);
>> +packet_trace(buf, size, 1);
>> +if (gentle) {
>> +ret = (
>> +!write_or_whine_pipe(fd, hdr, sizeof(hdr), "pkt-line 
>> header") ||
> 
> You can write '4' here, no need for sizeof(hdr)... though compiler would
> optimize it away.

Right, it would be optimized. However, I don't like the 4 there either. OK to 
use a macro
instead? PKTLINE_HEADER_LEN ?


>> +!write_or_whine_pipe(fd, buf, size, "pkt-line data")
>> +);
> 
> Do we want to try to write "pkt-line data" if "pkt-line header" failed?
> If not, perhaps De Morgan-ize it
> 
>  +ret = !(
>  +write_or_whine_pipe(fd, hdr, sizeof(hdr), "pkt-line 
> header") &&
>  +write_or_whine_pipe(fd, buf, size, "pkt-line data")
>  +);


Original:
ret = (
!write_or_whine_pipe(fd, hdr, sizeof(hdr), "pkt-line 
header") ||
!write_or_whine_pipe(fd, data, size, "pkt-line data")
);

Well, if the first write call fails (return == 0), then it is negated and 

Re: [PATCH v3 06/10] run-command: add clean_on_exit_handler

[Lars Schneider]

> On 30 Jul 2016, at 11:50, Johannes Sixt  wrote:
> 
> Am 30.07.2016 um 01:37 schrieb larsxschnei...@gmail.com:
>> Some commands might need to perform cleanup tasks on exit. Let's give
>> them an interface for doing this.
>> 
>> Signed-off-by: Lars Schneider 
>> ---
>> run-command.c | 12 
>> run-command.h |  1 +
>> 2 files changed, 9 insertions(+), 4 deletions(-)
>> 
>> diff --git a/run-command.c b/run-command.c
>> index 33bc63a..197b534 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -21,6 +21,7 @@ void child_process_clear(struct child_process *child)
>> 
>> struct child_to_clean {
>>  pid_t pid;
>> +void (*clean_on_exit_handler)(pid_t);
>>  struct child_to_clean *next;
>> };
>> static struct child_to_clean *children_to_clean;
>> @@ -30,6 +31,8 @@ static void cleanup_children(int sig, int in_signal)
>> {
>>  while (children_to_clean) {
>>  struct child_to_clean *p = children_to_clean;
>> +if (p->clean_on_exit_handler)
>> +p->clean_on_exit_handler(p->pid);
> 
> This summons demons. cleanup_children() is invoked from a signal handler. In 
> this case, it can call only async-signal-safe functions. It does not look 
> like the handler that you are going to install later will take note of this 
> caveat!
> 
>>  children_to_clean = p->next;
>>  kill(p->pid, sig);
>>  if (!in_signal)
> 
> The condition that we see here in the context protects free(p) (which is not 
> async-signal-safe). Perhaps the invocation of the new callback should be 
> skipped in the same manner when this is called from a signal handler? 
> 507d7804 (pager: don't use unsafe functions in signal handlers) may be worth 
> a look.

Thanks a lot of pointing this out to me!

Do I get it right that after the signal "SIGTERM" I can do a cleanup and don't 
need to worry about any function calls but if I get any other signal then I can 
only perform async-signal-safe calls?

If this is correct, then the following solution would work great:

if (!in_signal && p->clean_on_exit_handler)
p->clean_on_exit_handler(p->pid);

Thanks,
Lars--
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 v6 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 v6 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 ec50932..9e527de 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->call_depth && 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 v6 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 died without flushing, losing accumulated output.  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..b972a83 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;
+
+   flush_output(o);
+   va_start(params, err);
+   strbuf_vaddf(>obuf, err, params);
+   va_end(params);
+   error("%s", o->obuf.buf);
+   strbuf_reset(>obuf);
+
+   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 conflict?"));
-   

[PATCH v6 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 v6 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 v6 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 | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 9e527de..c9e4dbc 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2069,8 +2069,10 @@ 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)
+   if (clean < 0) {
+   flush_output(o);
return clean;
+   }
 
if (o->call_depth) {
*result = make_virtual_commit(mrtree, "merged tree");
-- 
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 v6 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 v6 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 99c9635..ec50932 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);
}
@@ -35,12 +35,21 @@ static int err(struct merge_options *o, const char *err, 
...)
 {
va_list params;
 
-   flush_output(o);
+   if (o->buffer_output < 2)
+   flush_output(o);
+   else {
+   strbuf_complete(>obuf, '\n');
+   strbuf_addstr(>obuf, "error: ");
+   }
va_start(params, err);
strbuf_vaddf(>obuf, err, params);
va_end(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);
+   }
 
return -1;
 }
diff --git a/merge-recursive.h b/merge-recursive.h
index d415724..735343b 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; /* 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

[PATCH v6 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 v6 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 v6 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
assumed 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 b972a83..99c9635 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 v6 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 v6 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 v6 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 v6 00/16] Use merge_recursive() directly in the builtin am

[Johannes Schindelin]

This is the sixth 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 fifth iteration of this patch series:

- the commit message that talked about swallowing messages was improved

- the va_start()/va_end() statements were moved closer to the vararg usage

- the `buffer_output` field is no longer a bit field because it is now
  logically more like the verbosity level, which we traditionally keep
  as a full `int`

- the flush_output() call is no longer moved, but instead added to the
  code path when we return early

- the commit message saying that we speculated about stdout's buffer
  size now instead states that we made an assumption

This patch series touches rather important code. I 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, 419 insertions(+), 294 deletions(-)

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

 diff --git a/merge-recursive.c b/merge-recursive.c
 index 66e93e0..c9e4dbc 100644
 --- a/merge-recursive.c
 +++ b/merge-recursive.c
 @@ -35,21 +35,21 @@ static int err(struct merge_options *o, const char *err, 
...)
  {
va_list params;
  
 -  va_start(params, err);
if (o->buffer_output < 2)
flush_output(o);
else {
strbuf_complete(>obuf, '\n');
strbuf_addstr(>obuf, "error: ");
}
 +  va_start(params, err);
strbuf_vaddf(>obuf, err, params);
 +  va_end(params);
if (o->buffer_output > 1)
strbuf_addch(>obuf, '\n');
else {
error("%s", o->obuf.buf);
strbuf_reset(>obuf);
}
 -  va_end(params);
  
return -1;
  }
 @@ -2069,16 +2069,18 @@ 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)
 +  if (clean < 0) {
 +  flush_output(o);
return clean;
 +  }
  
if (o->call_depth) {
*result = make_virtual_commit(mrtree, "merged tree");
commit_list_insert(h1, &(*result)->parents);
commit_list_insert(h2, &(*result)->parents->next);
}
 -  if (o->buffer_output < 2)
 +  flush_output(o);
 +  if (!o->call_depth && o->buffer_output < 2)
strbuf_release(>obuf);
if (show(o, 2))
diff_warn_rename_limit("merge.renamelimit",
 diff --git 

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

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

Re: [PATCH v3 01/10] pkt-line: extract set_packet_header()

[Lars Schneider]

> On 30 Jul 2016, at 12:30, Jakub Narębski  wrote:
> 
> W dniu 30.07.2016 o 01:37, larsxschnei...@gmail.com pisze:
>> From: Lars Schneider 
>> 
>> set_packet_header() converts an integer to a 4 byte hex string. Make
>> this function locally available so that other pkt-line functions can
>> use it.
> 
> This description is not that clear that set_packet_header() is a new
> function.  Perhaps something like the following
> 
>  Extract the part of format_packet() that converts an integer to a 4 byte
>  hex string into set_packet_header().  Make this new function ...
> 
> I also wonder if the part "Make this [new] function locally available..."
> is needed; we need to justify exports, but I think we don't need to
> justify limiting it to a module.  If you want to justify that it is
> "static", perhaps it would be better to say why not to export it.
> 
> Anyway, I think it is worthy refactoring (and compiler should be
> able to inline it, so there are no nano-performance considerations).
> 
> Good work!

Thank you! I would go with this then:

Extract the part of format_packet() that converts an integer to a 4 byte
hex string into set_packet_header().

OK?


>> 
>> Signed-off-by: Lars Schneider 
>> ---
>> pkt-line.c | 15 ++-
>> 1 file changed, 10 insertions(+), 5 deletions(-)
>> 
>> diff --git a/pkt-line.c b/pkt-line.c
>> index 62fdb37..445b8e1 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])
> 
> I guess that this is inherited from the original, but this preprocessor
> macro is local to the format_header() / set_packet_header() function,
> and would not work outside it.  Therefore I think we should #undef it
> after set_packet_header(), just in case somebody mistakes it for
> a generic hex() function.  Perhaps even put it inside set_packet_header(),
> together with #undef.
> 
> But I might be mistaken... let's check... no, it isn't used outside it.

Agreed. Would that be OK?

static void set_packet_header(char *buf, const int size)
{
static char hexchar[] = "0123456789abcdef";
#define hex(a) (hexchar[(a) & 15])
buf[0] = hex(size >> 12);
buf[1] = hex(size >> 8);
buf[2] = hex(size >> 4);
buf[3] = hex(size);
#undef hex
}

- Lars--
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] pager: move pager-specific setup into the build

[brian m. carlson]

On Mon, Aug 01, 2016 at 10:57:02AM +0200, Jakub Narębski wrote:
> W dniu 01.08.2016 o 09:00, Eric Wong pisze:
> > "brian m. carlson"  wrote:
> >> So it looks like this function splits on spaces but doesn't provide any
> >> escaping mechanism.  Is there any case in which we want to accept
> >> environment variables containing whitespace?  I ask this as someone that
> >> has EDITOR set to "gvim -f" on occasion and seeing how tools sometimes
> >> handle that poorly.
> 
> This is to handle environment variables holding program options,
> which are usually (but possibly not often) using single character
> options bundled together, that is, not using spaces.
> 
> Moreover, it is about holding program options to pager.

I understand that.  My point is that we should consider corner cases
like how we're going to handle spaces.

> > Yes, it's only split on spaces right now.  While I don't think
> > there's any current case where spaces would be useful/desirable;
> > I suppose a 3rd patch in this series could add support for using
> > split_cmdline (from alias.c)...
> 
> Is there any pager that needs spaces in options-set environment
> variable?  Does MORE allow option bundling?

We seem to accept GIT_PAGER="par | less" and par definitely accepts
spaces in its environment variables.  That seems to be a corner case,
though, and I haven't seen par practically used in years.

We may also want to consider EXINIT for people who pipe to vi.  Again,
I'm not sure this is very common; most people would use an .exrc or
.vimrc.

I'd say if we can't come up with any better examples, I'd skip handling
it for now.  I'll try to come up with a patch to add it later.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: git bisect for reachable commits only

[Oleg Taranenko]

Guys,

further investigation shows, git bisect is broken from its core... really.
Let consider 3rd a bit more complicated scenario

#cd ..
#rm -rf bisect3
mkdir bisect3
cd bisect3
git init
git touch coffee
git commit -am "create coffee"
git branch tee
echo sugar >> coffee
git commit -am "add sugar"  # we are still in master branch
echo "milk" >> coffee
git commit -am "milk for coffee"
ex +g/sugar/d -cwq coffee  # introducing 'bug'
git commit -am "somehow remove sugar"
echo "mixing..." >> coffee
git commit -am "coffee mixing"

git checkout tee# get back to coffee without sugar
git touch tee
git commit -am "tee"

git branch cocktail
echo "sugar" >> tee
git commit -am "sugar for tee"
echo "milk" >> tee
git commit -am "milk for tee"
echo "mixing..." >> tee
git commit -am "tee mixing"

git checkout cocktail
git touch cocktail
git commit -am "prepare cocktail"
echo orange >>cocktail
git commit -am "add orange juice"
echo rum >>cocktail
git commit -am "add rum"
echo mixing >> cocktail
git commit -am "cocktail mixing"
cat cocktail  #orange, rum, mixing
git merge tee
git merge master

git touch serve
git commit -am "serving..."

git log --full-history --graph --pretty=oneline

* 059adf903a2cbc06fe05dda4c916e2c586907f23 serving...
*   efc89d5253d3126defc7362c25ef069ae9b43fc7 Merge branch 'master' into cocktail
|\
| * dd41e230a3cac5d51a1e994747ff470e2af03cae coffee mixing
| * c2a44672f1197f34e04cd0fd66434a2b286b574e somehow remove sugar
| * f50352cfb6bc4a237b73c95ed7ebca074603ae11 milk for coffee
| * 79b253b316cdc3668697afe473610e35b453ab2f add sugar
* |   2d626eb5cfaa40a4503be58a5ed27f1ececa6d02 Merge branch 'tee' into cocktail
|\ \
| * | 7aba690c6c6f73f1906871c9dbf9737ec11a152b tee mixing
| * | eca611a93697359ec7a52f4a045461180bc365c3 milk for tee
| * | 7d6844724d0e81751ec1a67c1ffdf0d0fb932350 sugar for tee
* | | 6754e816922989d5870ec3452437bbbe6aca4d0f cocktail mixing
* | | 5cbbf0f0882c497590838b163210db3a393b647e add rum
* | | b46d7d8a361daae382fbef7acabda5416d23da46 add orange juice
* | | e571fdd09582e40fc54ffc5a4f112eac2b9f2c8e prepare cocktail
|/ /
* | 041a5a53704bccc60c489f8c9a4742bad79d5a95 tee
|/
* a52a4fa6770d000a9f4e9e297739a6dc88c0cc50 create coffee

As you can see, no tricks with amended history, but...

git bisect start HEAD 79b2
Bisecting: 8 revisions left to test after this (roughly 3 steps)
[6754e816922989d5870ec3452437bbbe6aca4d0f] cocktail mixing
cat coffee
git bisect bad
Bisecting: 2 revisions left to test after this (roughly 1 step)
[e571fdd09582e40fc54ffc5a4f112eac2b9f2c8e] prepare cocktail
git bisect bad
Bisecting: 0 revisions left to test after this (roughly 0 steps)
[041a5a53704bccc60c489f8c9a4742bad79d5a95] tee
git bisect bad
041a5a53704bccc60c489f8c9a4742bad79d5a95 is the first bad commit
commit 041a5a53704bccc60c489f8c9a4742bad79d5a95
Author: Oleg Taranenko 
Date:   Mon Aug 1 10:53:52 2016 +0200

tee

:00 100644 
e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 A tee

git bisect ever not looked into the path where good commit is
declared. Instead it found somehow most common ancestor from whole
tree (a52a create coffee),  assume it is GOOD commit (why !?) and
check only ^1 (not ^2) parent commit for testing as a potential bug
commit.
No wonder now, I got a disaster result, looking in my heavy enterprise
repository.

Can somebody take care of this issue?

Thanks, Oleg

On Sun, Jul 31, 2016 at 2:06 AM, Oleg Taranenko  wrote:
> Hi Junio,
>
> Thanks for reply.
> Let consider two pretty similar use cases.
>
>  SCENARIO 1 
>
> mkdir bisect
> cd bisect/
> git init
> git touch coffee
> git commit -am "create coffee"
> git branch develop
> echo sugar >> coffee
> git commit -am "add sugar" # we are still in master branch
> git checkout develop   # get back to coffe without sugar
> git touch tee  # cooking tee in develop branch
> git commit -am "tee"
> git merge master   #
> cat coffee # after merge coffe has sugar
> ex +g/sugar/d -cwq coffee  # introducing 'bug' by removing sugar from coffee
> git commit -am "merged/amended" --amend   # the history is amended
> echo "sugar" >> tee
> git commit -am "sugar for tee"  # just advance for measure
>
> # -- We are getting following state --
> git status # develop branch
> git log --full-history --graph --pretty=oneline
> * 83e9577b4a5d553fdc16806fdea9757229ea9222 sugar for tee
> *   23a4aa69a9d5c03aa14584400b7ee00c4d63 merged/amended
> |\
> | * 4c1caf7cb2417181c035a953afdf2389dd130aef add sugar
> * | c080fb4df39d721e2f2e0fdd91fe16d8bdd77515 tee
> |/
> * 3c3043b7d0a0d260c78db55b565f26e430aa5c80 create coffee
>
> cat coffee # nothing# discovering coffee has no sugar
> git checkout 4c1c   # but we remember it should to 
> have
> cat coffee  # ..."sugar"