Re: [PATCH v3] pager: move pager-specific setup into the build

[Jeff King]

On Thu, Aug 04, 2016 at 03:43:01AM +, Eric Wong wrote:

> +PAGER_ENV_CQ = "$(subst ",\",$(subst \,\\,$(PAGER_ENV)))"
> +PAGER_ENV_CQ_SQ = $(subst ','\'',$(PAGER_ENV_CQ))
> +BASIC_CFLAGS += -DPAGER_ENV='$(PAGER_ENV_CQ_SQ)'

Here we set up CQ_SQ, but there is no PAGER_ENV_SQ.

And then...

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

Here we depend on writing PAGER_ENV_SQ, which will be blank (and
git-sh-setup is broken as a result).

> diff --git a/config.mak.uname b/config.mak.uname
> index 17fed2f..b232908 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=FRX
>  endif

Is it worth setting up PAGER_ENV's default values before including
config.mak.*, and then using "+=" here? That avoids this line getting
out of sync with the usual defaults.

> +static void setup_pager_env(struct argv_array *env)
> +{

I know you said you don't like string parsing in C. Here is a patch (on
top of yours) that converts the parsing to shell, and generates a
pre-built array-of-struct (this is similar to the big series I posted
long ago, but just touching this one spot, not invading the whole
Makefile). Feel free to ignore it as over-engineered, but I thought I'd
throw it out there in case it appeals.

-- >8 --
diff --git a/.gitignore b/.gitignore
index 05cb58a..b0fd5ec 100644
--- a/.gitignore
+++ b/.gitignore
@@ -180,6 +180,7 @@
 /gitweb/static/gitweb.js
 /gitweb/static/gitweb.min.*
 /common-cmds.h
+/pager-env.h
 *.tar.gz
 *.dsc
 *.deb
diff --git a/Makefile b/Makefile
index 0b36b5e..2e009cd 100644
--- a/Makefile
+++ b/Makefile
@@ -680,6 +680,7 @@ XDIFF_LIB = xdiff/lib.a
 VCSSVN_LIB = vcs-svn/lib.a
 
 GENERATED_H += common-cmds.h
+GENERATED_H += pager-env.h
 
 LIB_H = $(shell $(FIND) . \
-name .git -prune -o \
@@ -1641,9 +1642,7 @@ 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)'
+PAGER_ENV_SQ = $(subst ','\'',$(PAGER_ENV))
 
 ALL_CFLAGS += $(BASIC_CFLAGS)
 ALL_LDFLAGS += $(BASIC_LDFLAGS)
@@ -1767,6 +1766,10 @@ common-cmds.h: generate-cmdlist.sh command-list.txt
 common-cmds.h: $(wildcard Documentation/git-*.txt)
$(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh command-list.txt >$@+ 
&& mv $@+ $@
 
+pager-env.h: generate-pager-env.sh GIT-BUILD-OPTIONS
+   $(QUIET_GEN)$(SHELL_PATH) ./generate-pager-env.sh '$(PAGER_ENV_SQ)' 
>$@+ && \
+   mv $@+ $@
+
 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):$(PAGER_ENV)
diff --git a/generate-pager-env.sh b/generate-pager-env.sh
new file mode 100644
index 000..5b67b59
--- /dev/null
+++ b/generate-pager-env.sh
@@ -0,0 +1,28 @@
+#!/bin/sh
+
+c_quote () {
+   printf '%s' "$@" | sed 's/\\//g; s/"/\\"/g;'
+}
+
+cat <<\EOF &&
+#ifndef PAGER_ENV_H
+#define PAGER_ENV_H
+
+static struct pager_env {
+   const char *key;
+   const char *value;
+} pager_env[] = {
+EOF
+
+# $* does whitespace splitting
+for i in $*; do
+   key=${i%%=*}
+   value=${i#*=}
+   printf '\t{ "%s", "%s" },\n' "$(c_quote "$key")" "$(c_quote "$value")"
+done &&
+
+cat <<\EOF
+};
+
+#endif /* PAGER_ENV */
+EOF
diff --git a/pager.c b/pager.c
index 6470b81..03c1a4a 100644
--- a/pager.c
+++ b/pager.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "run-command.h"
 #include "sigchain.h"
+#include "pager-env.h"
 
 #ifndef DEFAULT_PAGER
 #define DEFAULT_PAGER "less"
@@ -65,29 +66,12 @@ const char *git_pager(int stdout_is_tty)
 
 static void setup_pager_env(struct argv_array *env)
 {
-   const char **argv;
int i;
-   char *pager_env = xstrdup(PAGER_ENV);
-   int n = split_cmdline(pager_env, );
-
-   if (n < 0)
-   die("malformed build-time PAGER_ENV: %s",
-   split_cmdline_strerror(n));
-
-   for (i = 0; i < n; i++) {
-   char *cp = strchr(argv[i], '=');
-
-

Re: [PATCH v2] t4130: work around Windows limitation

[Johannes Sixt]

Am 03.08.2016 um 17:50 schrieb Junio C Hamano:

Johannes Sixt  writes:
The patch itself seems to got whitespace damaged somewhere
between you and me, which I fixed up,


Sorry for the damaged patch. I forgot that Thunderbird's "Send again" 
feature as well as copying and pasting between Thunderbird windows is 
poison for patch text.


What you have queued in js/t4130-rename-without-ino is correct. Thank 
you very much!


-- Hannes

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

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

[Jacob Keller]

On Wed, Aug 3, 2016 at 3:36 PM, Michael Haggerty  wrote:
> On 08/04/2016 12:29 AM, Jacob Keller wrote:
>> On Wed, Aug 3, 2016 at 3:00 PM, Michael Haggerty  
>> wrote:
>> It seems odd to be that a line with "199" spaces and nothing else will
>> return "-1" but a line with 200 spaces and nothing else will return
>> 200..? Would it be safe to just return -1 in both cases (if a line is
>> all spaces or starts with more than 200 spaces just return -1)?
>>
>>> +}
>>> +
>
> Thanks for your feedback.
>
> I was implicitly assuming that such lines would have text somewhere
> after those 200 spaces (or 25 TABs or whatever). But you're right, the
> line could consist only of whitespace. Unfortunately, the only way to
> distinguish these two cases is to read the rest of the line, which is
> exactly what we *don't* want to do.
>
> But I think it doesn't matter anyway. Such "text" will likely never be
> read by a human, so it's not a big deal if the slider position is not
> picked perfectly. And remember, this whole saga is just to improve the
> aesthetics of the diff. The diff is *correct* (e.g., in the sense of
> applicable) regardless of where we position the sliders.
>
> Michael
>

I think in this case treating it as "all whitespace" is more natural
than treating it as "200 characters with something following it"
because the only thing we've found so far is all white space.

Either way it's not really a big deal here.

Thanks,
Jake
--
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: [TIG PATCH] test: make diff/log work with git 2.9

[Jonas Fonseca]

On Tue, Jun 14, 2016 at 4:34 AM, Michael J Gruber
 wrote:
> git 2.9.0 switches the default for diff.renames to true.
>
> Set this to false in config so that the test suite runs unmodified for
> old and new git.

Thanks!

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

[PATCH v3] 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.  This allows us to
set a better default for FreeBSD more(1), which misbehaves if
MORE environment variable is left empty, but accepts the same
variables as less(1)

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

Signed-off-by: Eric Wong 
---
  v3 changes (from v2, d3aed319c9abac006060bc81e865c93ff8363066)
  - Squashed git-sh-setup quoting fix from Junio
  - set MORE=FRX on FreeBSD to match LESS
  - simplify setup_pager_env to avoid leaks using split_cmdline

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

  for you to fetch changes up to 18c69fb16d866f42b53242142229de4801964d37:

pager: move pager-specific setup into the build (2016-08-04 03:17:16 +)

  
  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  | 32 
   4 files changed, 53 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..b232908 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=FRX
 endif
 ifeq ($(uname_S),OpenBSD)
NO_STRCASESTR = YesPlease
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 0c34aa6..a8a4576 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..6470b81 100644
--- a/pager.c
+++ b/pager.c
@@ -63,14 +63,38 @@ const char *git_pager(int stdout_is_tty)
return pager;
 }
 
+static void setup_pager_env(struct argv_array *env)
+{
+   const char **argv;
+   int i;
+   

Re: [[PATCH v2] 4/4] rebase: avoid computing unnecessary patch IDs

[Junio C Hamano]

Junio C Hamano  writes:

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

How about this one instead (to be squashed into 4/4)?

The updated wording directly addresses the puzzlement I initially
felt "This returns error() which is always negative, so comparing
(A, B) would say A < B, while comparing (B, A) would say B < A.
Would it cause a problem in the caller?" while reading the function
by being explicit that the sign does not matter.

 patch-ids.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/patch-ids.c b/patch-ids.c
index 0a4828a..082412a 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -16,6 +16,16 @@ int commit_patch_id(struct commit *commit, struct 
diff_options *options,
return diff_flush_patch_id(options, sha1, diff_header_only);
 }
 
+/*
+ * When we cannot load the full patch-id for both commits for whatever
+ * reason, the function returns -1 (i.e. return error(...)). Despite
+ * the "cmp" in the name of this function, the caller only cares about
+ * the return value being zero (a and b are equivalent) or non-zero (a
+ * and b are different), and returning non-zero would keep both in the
+ * result, even if they actually were equivalent, in order to err on
+ * the side of safety.  The actual value being negative does not have
+ * any significance; only that it is non-zero matters.
+ */
 static int patch_id_cmp(struct patch_id *a,
struct patch_id *b,
struct diff_options *opt)
--
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: What's cooking (interim report)

[Jeff King]

On Wed, Aug 03, 2016 at 04:06:20PM -0700, Junio C Hamano wrote:

> * jk/parseopt-string-list (2016-08-03) 1 commit
>  - blame: drop strdup of string literal
> 
>  A recent API change to parse_opt_string_list() introduced a small
>  unintended memory leak in the command line parsing of "git blame",
>  which has been plugged.
> 
>  Will merge to 'next'.

Actually, the leak was always there. It's just that
parse_opt_string_list used to leak _too_, and it was fixed, but this
case left behind.

Not that it matters too much. I doubt this one is exciting enough to
make it into the release notes. :)

> * jk/pack-objects-optim-mru (2016-07-29) 1 commit
>  - pack-objects: use mru list when iterating over packs
>  (this branch uses jk/pack-objects-optim.)
> 
>  This is still questionable in that it can attempt to create a cycle
>  in delta-chain, only to be stopped by the last-ditch recovery logic
>  in there.

I've been doing some thinking and experimenting on this. I'll try to
write up the current status tonight.

-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

Hello Friend

[emelda]

Hello Friend

How is life been with you and your present health condition i hope you are 
doing great? i saw your profile and i like it so i decided to contact you for a 
possible relationship and other good business/investment i want to discuss with 
you,remember that age,race nor ethnic will not be a barrier between us , i am 
Emelda 23yrs old, and very good looking,i will tell you more about my self when 
i read back from you,

with love
Emelda
--
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

refactoring a branch with e.g. meld how?

[Britton Kerin]

I've been asked to seriously reorganize and compress a big branch (75 commits)

What I'd like to do is make branches foo_flattened with all commits
and branch foo_reorganized
starting at the ancestor, then work by incrementally doing
meld/commit/meld/commit/etc from foo_flattened into foo_reorganized.

I hoped to use git difftool -d foo_flattened foo_reorganized, taking
advantage of the symlink behavior on the right hand side, but it links
the individual files not the entire working tree, which isn't too
useful since a lot of the changes introduce files.

Is there a git command to help with this or should I just be making a
copy of the entire foo_flattened tree and work from that?

I recall running across some command that I think was supposed to let
you have coexisting working trees of multiple commits or something,
but I can't find it now.

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

[Jakub Narębski]

[Some of those answers might have been invalidated by v4]

W dniu 01.08.2016 o 19:55, Lars Schneider pisze:
>> On 01 Aug 2016, at 00:19, Jakub Narębski  wrote:
>> W dniu 30.07.2016 o 01:38, larsxschnei...@gmail.com pisze:
>> [...]

>>> +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!
 
Errr... what? multi_packet_read() is the current name...
 
>> 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.

True.  

BTW. this means that it cannot share code with fetch / push codebase,
where Git spools from pkt-line to packfile on disk.


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

Well, if current code doesn't have such check, then I think it would
be all right to not have it either.

Note that we do not use C++ comments.
 
 

>>> +
>>> +   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 boolean expression with 'return'.
>> But I am still unsure if it is good API, this return value.
> 
> According to Peff zero for success is the usual style:
> http://public-inbox.org/git/20160728133523.GB21311%40sigill.intra.peff.net/

The usual case is 0 for success, but -1 (and not 1) for error.
But I agree with Peff that keeping existing API is better. 

>>> +   );
>>> +   strbuf_setlen(sb, total_bytes_read);
>>> +   return (is_stream ? 0 : expected_bytes != total_bytes_read);
>>> +}
>>> +
>>> +static int multi_packet_write_from_fd(const int fd_in, const int fd_out)
>>
>> Is it equivalent of copy_fd() function, but where destination uses pkt-line
>> and we need to pack data into pkt-lines?
> 
> Correct!

Yes, and we cannot keep the naming convention.  Though maybe mentioning
the equivalence in the comment above function would be good idea...

>>> +   return did_fail;
>>
>> Return true on fail?  Shouldn't we follow example of copy_fd()
>> from copy.c, and return COPY_READ_ERROR, or COPY_WRITE_ERROR,
>> or PKTLINE_WRITE_ERROR?
> 
> OK. How about this?
> 
> static int multi_packet_write_from_fd(const int fd_in, const int fd_out)
> {
>   int did_fail = 0;
>   ssize_t bytes_to_write;
>   while (!did_fail) {
>   bytes_to_write = xread(fd_in, 
> PKTLINE_DATA_START(packet_buffer), PKTLINE_DATA_MAXLEN);
>   if (bytes_to_write < 0)
>   return COPY_READ_ERROR;
>   if (bytes_to_write == 0)
>   break;
>   did_fail |= direct_packet_write(fd_out, packet_buffer, 
> PKTLINE_HEADER_LEN + bytes_to_write, 1);
>   }
>   if (!did_fail)
>   did_fail = packet_flush_gently(fd_out);
>   return (did_fail ? COPY_WRITE_ERROR : 0);
> }

That's better, I think. 
 
>>> +}
>>> +
>>> +static int multi_packet_write_from_buf(const char *src, size_t len, int 
>>> fd_out)
>>
>> It is equivalent of write_in_full(), with different order of parameters,
>> but where destination file descriptor expects pkt-line and we need to pack
>> data into pkt-lines?
> 
> True. Do you suggest to reorder parameters? I also would like to rename `src` 
> to `src_in`, OK?

Well, no need to reorder parameters.  Better keep it the same as for
other function.  'src' is input ('source'), 'src_in' is tautologic.

>> 

Re: [RFC/PATCH] rebase--interactive: Add "sign" command

[Chris Packham]

On Thu, Aug 4, 2016 at 6:08 AM, Jeff King  wrote:
> On Wed, Aug 03, 2016 at 09:08:48AM -0700, Junio C Hamano wrote:
>
>> > However, I could imagine that we actually want this to be more extensible.
>> > After all, all you are doing is to introduce a new rebase -i command that
>> > does nothing else than shelling out to a command.
>>
>> Yup, I tend to agree.
>>
>> Adding "sign" feature (i.e. make it pass -S to "commit [--amend]")
>> may be a good thing, but adding "sign" command to do so is not a
>> great design.
>
> I'm not sure what you mean by "feature" here, but it reminded me of
> Michael's proposal to allow options to todo lines:
>
>   http://public-inbox.org/git/530da00e.4090...@alum.mit.edu/
>
> which would allow:
>
>   pick -S 1234abcd
>
> If that's what you meant, I think it is a good idea. :)

That would definitely suit me. I see there was some discussion of
which options were sensible to support. --signoff and --reset-author
would be a good start from my point of view. Maybe that's something
that could be build on top of Johannes work.

>
> -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/PATCH] rebase--interactive: Add "sign" command

[Chris Packham]

On Thu, Aug 4, 2016 at 2:31 AM, Johannes Schindelin
 wrote:
> Hi Chris,
>
> On Wed, 3 Aug 2016, Chris Packham wrote:
>
>> This is similar to the existing "reword" command in that it can be used
>> to update the commit message the difference is that the editor presented
>> to the user for the commit. It provides a useful shorthand for "exec git
>> commit --amend --no-edit -s"



> Having said that, this patch clashes seriously with my current effort to
> move a lot of the interactive rebase from shell into plain C. It is
> actually ready, but getting this into the code base is really slow-going,
> unfortunately.
>
> Now, after looking at your patch it looks to me as if this would be easily
> ported, so there is not a big deal here.

Yeah sorry. I knew there was something in flight but ended up doing a
quick hack on top of master.

> However, I could imagine that we actually want this to be more extensible.
> After all, all you are doing is to introduce a new rebase -i command that
> does nothing else than shelling out to a command. Why not introduce a much
> more flexible feature, where you add something like "rebase -i aliases"?
>
> Maybe something like this:
>
> [rebase "command"]
> sign = git commit --amend -s --no-post-rewrite --no-edit -S

I did briefly consider that. I ended up taking the shortcut because I
had a patch series I needed to sign.

Elsewhere in this thread the idea of pick -S or reword -S was raised.
I'd actually prefer that because there seems little between 'git
config rebase.command.sign blah' and 'exec ~/sign.sh'

> I have not completely thought this through, but maybe this direction would
> make the interactive rebase even more powerful?

The uses I can think of are adding sign-off and running "make check".
For me rebase -i doesn't need to be much more powerful than that.
--
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: Client exit whilst running pre-receive hook : commit accepted but no post-receive hook ran

[Stephen Morton]

On 2016-08-03 3:54 PM, Junio C Hamano wrote:

Jeff King  writes:


I agree it would be a good property to have. I think it's hard to do
atomically, though. Certainly we can wait to tell the other side "your
push has been recorded" until after the hook is run. But we would
already have updated the refs locally at that point (and we must -- that
is part of the interface to the post-receive hooks, that the refs are
already in place). So would we roll-back the ref update then? Even that
suffers from power failures, etc.

So I'm not sure if making it truly atomic is all the feasible.

As long as the requirement is that post- hook must see the updated
ref in place, I do not think it is feasible to give "the hook always
runs once" guarantee, without cooperation by other parts of the flow
(think: pulling the power at an arbitrary point in the process).

A receiving repository can implement it all in the userland, I would
think, though:

  * A pre-receive hook records the intention to update a ref (from
what old commit to what new commit), and does not return until
that record is securely in a database;

  * A post-receive hook checks the entry in the database above (it
_must_ find one), and atomically does its thing and marks the
entry "done";

  * A separate sweeper scans the database for entries not yet marked
as "done", sees if the ref has been already updated, and
atomically does its thing and marks the entry "done" (the same
can be done as part of a post-receive for previously pushed
commit that pre-receive recorded but did not manage to run
post-receive before the power was pulled or the user did \C-c).

As you originally described, the non-atomicity is not new; as long
as we have necessary hooks in place on the git-core side for
repositories that want a stronger guarantee, I do not think there is
any more thing we need to do on this topic.  If we can narrow the
window in a non-intrusive way, that would be a good thing to do,
though.



I certainly understand not being able to make it atomic when faced with 
say "pulling the power at an arbitrary point in the process". That seems 
to me almost along the lines of disaster recovery contingency plans. But 
could we not guarantee that if there is no problem on the receiving end, 
that "IF a commit is received and the ref updated, THEN the post-receive 
hook is guaranteed to run".


The not-so-uncommon situation where I saw this was where a user had 
second-thoughts and hit Ctrl-C in the middle of a push. The push went 
through --the ref was updated-- but the post-receive hooks were not run.


Steve



--
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/8] xdl_change_compact(): clarify code

[Stefan Beller]

On Wed, Aug 3, 2016 at 4:14 PM, Michael Haggerty  wrote:
> On 08/04/2016 12:11 AM, Stefan Beller wrote:
>> On Wed, Aug 3, 2016 at 3:00 PM, Michael Haggerty  
>> wrote:
>>> Write things out a bit longer but less cryptically. Add more comments.
>>
>> By less cryptic you mean in Git coding style ;)
>> The original author (do we want to cc Davido?) may disagree.
>
> Davide hasn't contributed since 2008 and libxdiff is not being
> developed, so I didn't think he'd be interested.

ok.

>
> Yes, tastes certainly differ. If more people like the old version
> better, I will gnash my teeth and undo these "clarification" patches.

I was not asking for undoing these, but giving short cryptic answers myself. ;)
While I agree the variable names are way better than before, the use of while
instead of for (and then doing another final ++ after the loop) extended some
one liners to about 5. I am totally fine with that as they are easier
to read for me
as I understand them as Git style, hence easier to read.

There may be old timers (who have knowledge of C from other projects), that
would prefer the style as before:

e.g.

-   start = i;
-   for (i++; rchg[i]; i++);
-   for (; rchgo[io]; io++);
+   start = i++;
+
+   while (rchg[i])
+   i++;
+
+   while (rchgo[io])
+  io++;

This doesn't change variable names, but it only transforms a for loop with no
body in a more readable structure of while loops separated by white space.
So for such a chunk I could imagine people arguing about adding lines of code
(which is valuable screen real estate) for only slight gain if any.
I am not one of these.


> I
> mean, what's not to like about variable names like "grpsiz" and "ixref"?

faster typng ;)


>
>>> +
>>> +   /*
>>> +* Are there any blank lines that could appear as 
>>> the last
>>> +* line of this group?
>>> +*/
>>
>> IIRC this comment is not quite correct as this 'only' counts the number of
>> blank lines within the forward shifting section, i.e. in the movable space.
>>
>> Later we use it as a boolean indicator (whether or not it is equal to 0)
>> to see if we can do better.
>>
>> Any other change in code and comments looks good to me, but this stood out
>> like a sore thumb. (Probably the old heuristic as a whole stood out, but the
>> comment here specifically sounds /wrong/ to me in this place. How can
>> a question document a variable? I'd rather expect a question comment
>> to ease the understanding of a condition)
>
> I don't understand your objection. A blank line can appear as the last
> line of the group if and only if it is within the shift range ("movable
> space") of the group, right? So it seems like our formulations are
> equivalent.

Sure, e.g. in 0fe5043da (2016-06-17, dir_iterator: new API for iterating
over a directory tree), struct dir_iterator_int we have a member

struct dir_iterator_int {
...
 /*
 * The number of levels currently on the stack. This is always
 * at least 1, because when it becomes zero the iteration is
 * ended and this struct is freed.
 */
 size_t levels_nr;
...
};

you could have written that comment as

/* How many levels do we have to free? */

but that would be misleading the same way as here.

I think a comment should carry useful information that is not
obvious from the code. So in this comment we want to convey the
message that we need to count blank lines to apply a heuristic later
on. Maybe

  /* Number of blank lines in the sliding area of the group */

as that

* states the actual thing we do
* doesn't hint at one particular intended use case later on
* it assumes you know what a "sliding area" is though.

I think what triggered me on questioning this comment, was the fact
that it is a question as we rarely have comments stated as questions.




>
> Since the variable is used as a boolean, it seemed natural to document
> it by stating the question that the true/false value is the answer to.

Oh I see. Another example (that maybe looks constructed) is the comment
of S_IFGITLINK in cache.h which is not "Is this entry a submodule?" but
rather some sentence of what a git link actually is. (though very short)

>
> If you have a concrete suggestion for a better comment, please let me know.

I'd go with the imperative form,

 /* Number of blank lines in the sliding area of the group */

if that makes sense to you?

Sorry for the bike shedding and not focusing on the real issues,

Stefan

>
> Michael
>
--
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 8/8] diff: improve positioning of add/delete blocks in diffs

[Stefan Beller]

On Wed, Aug 3, 2016 at 4:30 PM, Michael Haggerty  wrote:

> This is an important point for the optimization, but less so for the
> implementation of the heuristic here. I was dynamically optimizing the
> ten other variables, and everything that goes into the bonus includes
> one of those factors. If I had also let this factor of 10 vary, then the
> net behavior of the algorithm would be completely unchanged if I would,
> say, double all eleven parameters. This is bad for optimization, because
> (1) it increases the search space unnecessarily, and (2) it means that
> whole lines in the parameter space give identical behavior, making the
> algorithm waste time searching along those lines for a minimum.
>
>> So another way to write it
>> would be
>>
>> score - bonus/10;
>>
>> assuming the values of score and bonus are large enough.
>
> Score is the number of columns that some line is indented, so it can be
> 0 or 1 or any other positive integer. It is not "large enough", which is
> why the "10" can't be "1".

Right, I should have made it more clear that it was a hypothetical rewrite,
just to point out we are looking at only one of score or bonus.

>> but if I look at the boni definitions ranging from -63..50 they are scaled up
>> so much that the bonus may become larger than '1' unit of 'score', i.e.
>> it is not an epsilon any more. Or to put it another way:
>> If we were to s/10/100/ the results would be worse.
>
> If you would change s/10/100/ and simultaneously multiply the other
> constants by 10, the end results would be unchanged.

Right, so maybe a good name would be CONSTANT_SCALE_OF_ONE_INDENT
as it has the meaning that a bonus of 10 is equivalent of "one indent"
in the weighting.

Speaking of which, do we want to "over-optimize" to make that constant a
power of 2 as that is a supposedly faster multiplication?
(Just asking, feel free to reject; as I can imagine the numbers itself are
already magic, so why scale them with 42?^H^H^H 16?)

>
>> Rather the 10 describes the ratio of "advanced magic" to pure indentation
>> based scoring in my understanding.
>
> No, it's basically just a number against which the other constants are
> compared. E.g., if another bonus wants to balance out against exactly
> one space of indentation, its constant needs to be 10. If it wants to
> balance out against exactly 5 spaces, its constant needs to be 50. Etc.

So another interpretation is that the 10 gives the resolution for all other
constants, i.e. if we keep 10, then we can only give weights in 1/10 of
"one indent". But the "ideal" weight may not be a multiple of 1/10,
so we approximate them to the nearest multiple of 1/10.

If we were to use 1000 here, we could have a higher accuracy of the
other constants, but probably we do not care about the 3rd decimal place
for these because they are created heuristically from a corpus that may
not warrant a precision of constants with a 3rd decimal place.


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

Re: [PATCH v4 07/12] run-command: add clean_on_exit_handler

[Jeff King]

On Thu, Aug 04, 2016 at 01:09:57AM +0200, Lars Schneider wrote:

> > Or better yet, do not require a shutdown at all. The filter sees EOF and
> > knows there is nothing more to do. If we are in the middle of an
> > operation, then it knows git died. If not, then presumably git had
> > nothing else to say (and really, it is not the filter's business if git
> > saw an error or not).
> 
> EOF? The filter is supposed to process multiple files. How would one EOF
> indicate that we are done?

I think we may be talking about two different EOFs.

Git sends a file in pkt-line format, and the flush marks EOF for that
file. But the filter keeps running, waiting for more input. This can
happen multiple times.

Eventually git calls close() on the descriptor, and the filter sees the
"real" EOF (i.e., read() returns 0). That is the signal that git is
done.

> > I'm not sure if calling that "shutdown" makes sense, though. It's almost
> > more of a checkpoint (and I wonder if git would ever want to
> > "checkpoint" without hanging up the connection).
> 
> OK, I agree that the naming might not be ideal. But "checkpoint" does not
> convey that it is only executed once after all blobs are filtered?!

Does the filter need to care? It's told to do any deferred work, and to
report back when it's done. The fact that git is calling it before it
decides to exit is not the filter's business (and you can imagine for
something like fast-import, it might want to feed files to something
like LFS, too; it already checkpoints occasionally to avoid lost work,
and would presumably want to ask LFS to checkpoint, too).

> I understand that Git might not want to wait for the filter...

If git _doesn't_ want to wait for the filter, I don't think you need a
checkpoint at all. The filter just does its deferred work when it sees
git hang up the connection (i.e., the "real" EOF from above).

-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 8/8] diff: improve positioning of add/delete blocks in diffs

[Michael Haggerty]

On 08/04/2016 12:51 AM, Stefan Beller wrote:
> On Wed, Aug 3, 2016 at 3:41 PM, Michael Haggerty  wrote:
>> On 08/04/2016 12:30 AM, Stefan Beller wrote:
>>> On Wed, Aug 3, 2016 at 3:00 PM, Michael Haggerty  
>>> wrote:
>>>
 +return 10 * score - bonus;
>>>
>>> Would it make sense to define-ify the 10 as well
>>> as this is the only hardcoded number in the
>>> scoring function?
>>
>> I started answering this question by explaining why it is not important
>> to *optimize* the number 10 (namely because scores are only ever
>> compared against other scores, so an overall scaling factor makes no
>> difference). The factor 10 only has to be large enough to provide enough
>> dynamic range for the other (adjustable) parameters.
> 
> But it only scales the score, not the bonus.

Yes, that's how it provides the overall scale of the score. If it
multiplied both values, then it would be completely pointless.

This is an important point for the optimization, but less so for the
implementation of the heuristic here. I was dynamically optimizing the
ten other variables, and everything that goes into the bonus includes
one of those factors. If I had also let this factor of 10 vary, then the
net behavior of the algorithm would be completely unchanged if I would,
say, double all eleven parameters. This is bad for optimization, because
(1) it increases the search space unnecessarily, and (2) it means that
whole lines in the parameter space give identical behavior, making the
algorithm waste time searching along those lines for a minimum.

> So another way to write it
> would be
> 
> score - bonus/10;
> 
> assuming the values of score and bonus are large enough.

Score is the number of columns that some line is indented, so it can be
0 or 1 or any other positive integer. It is not "large enough", which is
why the "10" can't be "1".

If the calculations were done in floating point, then the factor could
be "1", because then the other factors could be made less than one.

> In some prior conversation you said you take the indent and add an epsilon
> for some special conditions to make one indent better than the other.
> 
> Epsilons are usually very small compared to the rest of the equation,

I should have mentioned that this heuristic is quite a bit more advanced
than my original proposal to use "indent" plus some "epsilon" factors.
The old discussion about epsilons is not relevant here except maybe as
an inspiration and starting point for this version.

> but if I look at the boni definitions ranging from -63..50 they are scaled up
> so much that the bonus may become larger than '1' unit of 'score', i.e.
> it is not an epsilon any more. Or to put it another way:
> If we were to s/10/100/ the results would be worse.

If you would change s/10/100/ and simultaneously multiply the other
constants by 10, the end results would be unchanged.

> Rather the 10 describes the ratio of "advanced magic" to pure indentation
> based scoring in my understanding.

No, it's basically just a number against which the other constants are
compared. E.g., if another bonus wants to balance out against exactly
one space of indentation, its constant needs to be 10. If it wants to
balance out against exactly 5 spaces, its constant needs to be 50. Etc.

Michael

--
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: [PATCHv4 6/7] submodule--helper: add remote-branch helper

[Stefan Beller]

On Wed, Aug 3, 2016 at 4:13 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index fb90c64..9be2c75 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -899,6 +899,39 @@ static int resolve_relative_path(int argc, const char 
>> **argv, const char *prefix
>>   return 0;
>
> I wonder who lost the leading SP before the tab here.  Will manually
> fix, so this is not a reason to force resending, but you may want to
> make sure there is no systemic cause to corrupt your future patches.

It's not a systematic issue, it was user error.
(I started writing out a story, but it got long)

Thanks,
Stefan

>
> 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 2/8] xdl_change_compact(): clarify code

[Michael Haggerty]

On 08/04/2016 12:11 AM, Stefan Beller wrote:
> On Wed, Aug 3, 2016 at 3:00 PM, Michael Haggerty  wrote:
>> Write things out a bit longer but less cryptically. Add more comments.
> 
> By less cryptic you mean in Git coding style ;)
> The original author (do we want to cc Davido?) may disagree.

Davide hasn't contributed since 2008 and libxdiff is not being
developed, so I didn't think he'd be interested.

Yes, tastes certainly differ. If more people like the old version
better, I will gnash my teeth and undo these "clarification" patches. I
mean, what's not to like about variable names like "grpsiz" and "ixref"?

>> +
>> +   /*
>> +* Are there any blank lines that could appear as 
>> the last
>> +* line of this group?
>> +*/
> 
> IIRC this comment is not quite correct as this 'only' counts the number of
> blank lines within the forward shifting section, i.e. in the movable space.
> 
> Later we use it as a boolean indicator (whether or not it is equal to 0)
> to see if we can do better.
> 
> Any other change in code and comments looks good to me, but this stood out
> like a sore thumb. (Probably the old heuristic as a whole stood out, but the
> comment here specifically sounds /wrong/ to me in this place. How can
> a question document a variable? I'd rather expect a question comment
> to ease the understanding of a condition)

I don't understand your objection. A blank line can appear as the last
line of the group if and only if it is within the shift range ("movable
space") of the group, right? So it seems like our formulations are
equivalent.

Since the variable is used as a boolean, it seemed natural to document
it by stating the question that the true/false value is the answer to.

If you have a concrete suggestion for a better comment, please let me know.

Michael

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

[Junio C Hamano]

Stefan Beller  writes:

> Gerrit has a "superproject subscription" feature[1], that triggers a
> commit in a superproject that is subscribed to its submodules.
> Conceptually this Gerrit feature can be done on the client side with
> Git via (except for raciness, error handling etc):

This would replace 7/7 not 6/7 obviously ;-)

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: [PATCHv4 6/7] submodule--helper: add remote-branch helper

[Junio C Hamano]

Stefan Beller  writes:

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index fb90c64..9be2c75 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -899,6 +899,39 @@ static int resolve_relative_path(int argc, const char 
> **argv, const char *prefix
>   return 0;

I wonder who lost the leading SP before the tab here.  Will manually
fix, so this is not a reason to force resending, but you may want to
make sure there is no systemic cause to corrupt your future patches.

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: What's cooking (interim report)

[Stefan Beller]

On Wed, Aug 3, 2016 at 4:06 PM, Junio C Hamano  wrote:
>
> * sb/submodule-update-dot-branch (2016-08-01) 7 commits
>  - submodule update: allow '.' for branch value
>  - submodule--helper: add remote-branch helper
>  - submodule-config: keep configured branch around
>  - submodule--helper: fix usage string for relative-path
>  - submodule update: narrow scope of local variable
>  - submodule update: respect depth in subsequent fetches
>  - t7406: future proof tests with hard coded depth
>
>  A few updates to "git submodule update".
>
>  Will merge to 'next'.

I sent out replacements for the two tip most commits
about 2 hours ago, hopefully they make it in before the merge to next,
otherwise I'll resend on top of these.

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

Re: [PATCH v4 07/12] run-command: add clean_on_exit_handler

[Lars Schneider]

> On 04 Aug 2016, at 00:53, Jeff King  wrote:
> 
> On Thu, Aug 04, 2016 at 12:15:46AM +0200, Lars Schneider wrote:
> 
>>> I'm not clear on why we want this cleanup filter. It looks like you use
>>> it in the final patch to send an explicit shutdown to any filters we
>>> start. But I see two issues with that:
>>> 
>>> 1. This shutdown may come at any time, and you have no idea what state
>>>the protocol conversation with the filter is in. You could be in
>>>the middle of sending another pkt-line, or in a sequence of non-command
>>>pkt-lines where "shutdown" is not recognized.
>> 
>> Maybe I am missing something, but I don't think that can happen because 
>> the cleanup callback is *only* executed if Git exits normally without error. 
>> In that case we would be in a sane protocol state, no?
> 
> OK, then maybe I am doubly missing the point. I thought this cleanup was
> here to hit the case where we call die() and git exits unexpectedly.
> 
> If you only want to cover the "we are done, no errors, goodbye" case,
> then why don't you just write shutdown when we're done?

I think I tried that at some point but the filter code is called from
multiple places and therefore I looked into atexit() (via run-command)
and it seemed easier. Do you have a place in mind where you would call 
the shutdown after all blobs are processed explicitly?


> I realize you may have multiple filters, but I don't think it should be
> run-command's job to iterate over them. You are presumably keeping a
> list of active filters, and should have a function to iterate over that.

Yes, that would be easy.


> Or better yet, do not require a shutdown at all. The filter sees EOF and
> knows there is nothing more to do. If we are in the middle of an
> operation, then it knows git died. If not, then presumably git had
> nothing else to say (and really, it is not the filter's business if git
> saw an error or not).

EOF? The filter is supposed to process multiple files. How would one EOF
indicate that we are done?


> Though...
> 
>> Thanks. The shutdown command is not intended to be a mechanism to tell
>> the filter that everything went well. At this point - as you mentioned -
>> the filter already received all data in the right way. The shutdown
>> command is intended to give the filter some time to perform some post
>> processing before Git returns.
>> 
>> See here for some brainstorming how this feature could be useful
>> in filters similar to Git LFS:
>> https://github.com/github/git-lfs/issues/1401#issuecomment-236133991
> 
> OK, so it is not really "tell the filter to shutdown" but "I am done
> with you, filter, but I will wait for you to tell me you are all done,
> so that I can tell the user".

Correct!


> I'm not sure if calling that "shutdown" makes sense, though. It's almost
> more of a checkpoint (and I wonder if git would ever want to
> "checkpoint" without hanging up the connection).

OK, I agree that the naming might not be ideal. But "checkpoint" does not
convey that it is only executed once after all blobs are filtered?!

I understand that Git might not want to wait for the filter...

- 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 4/7] trace: cosmetic fixes for error messages

[Jeff King]

The error messages for the trace code are often multi-line;
the first line gets a nice "warning:", but the rest are
left-aligned. Let's give them an indentation to make sure
they stand out as a unit.

While we're here, let's also downcase the first letter of
each error (our usual style), and break up a long line of
advice (since we're already using multiple lines, one more
doesn't hurt).

We also replace "What does 'foo' for GIT_TRACE mean?". While
cute, it's probably a good idea to give more context, and
follow our usual styles. So it's now "unknown trace value
for 'GIT_TRACE': foo".

Signed-off-by: Jeff King 
---
I went with an indent the size of "warning: ", because that string does
not actually get translated (it seems like it probably should, though).

I think it would be nicer to still to print:

 warning: first line
 warning: second line

etc. We do that for "advice:", but not the rest of the vreportf
functions. It might be nice to do that, but we'd have to go back to
printing into a buffer (since we can't break up the incoming format
string that we feed to fprintf).

 trace.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/trace.c b/trace.c
index 6a77e4d..4d68eb5 100644
--- a/trace.c
+++ b/trace.c
@@ -61,8 +61,8 @@ static int get_trace_fd(struct trace_key *key)
else if (is_absolute_path(trace)) {
int fd = open(trace, O_WRONLY | O_APPEND | O_CREAT, 0666);
if (fd == -1) {
-   warning("Could not open '%s' for tracing: %s\n"
-   "Defaulting to tracing on stderr...",
+   warning("could not open '%s' for tracing: %s\n"
+   " Defaulting to tracing on stderr...",
trace, strerror(errno));
key->fd = STDERR_FILENO;
} else {
@@ -70,11 +70,11 @@ static int get_trace_fd(struct trace_key *key)
key->need_close = 1;
}
} else {
-   warning("What does '%s' for %s mean?\n"
-   "If you want to trace into a file, then please set "
-   "%s to an absolute pathname (starting with /).\n"
-   "Defaulting to tracing on stderr...",
-   trace, key->key, key->key);
+   warning("unknown trace value for '%s': %s\n"
+   " If you want to trace into a file, then please 
set %s\n"
+   " to an absolute pathname (starting with /)\n"
+   " Defaulting to tracing on stderr...",
+   key->key, trace, key->key);
key->fd = STDERR_FILENO;
}
 
@@ -93,7 +93,7 @@ void trace_disable(struct trace_key *key)
key->need_close = 0;
 }
 
-static const char err_msg[] = "Could not trace into fd given by "
+static const char err_msg[] = "could not trace into fd given by "
"GIT_TRACE environment variable";
 
 static int prepare_trace_line(const char *file, int line,
-- 
2.9.2.670.g42e63de

--
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 5/7] trace: correct variable name in write() error message

[Jeff King]

Our error message for write() always mentions GIT_TRACE,
even though we may be writing for a different variable
entirely. It's also not quite accurate to say "fd given by
GIT_TRACE environment variable", as we may hit this error
based on a filename the user put in the variable (we do
complain and switch to stderr if the file cannot be opened,
but it's still possible to hit a write() error on the
descriptor later).

So let's fix those things, and switch to our more usual
"unable to do X: Y" format for the error.

Signed-off-by: Jeff King 
---
 trace.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/trace.c b/trace.c
index 4d68eb5..4efb256 100644
--- a/trace.c
+++ b/trace.c
@@ -93,9 +93,6 @@ void trace_disable(struct trace_key *key)
key->need_close = 0;
 }
 
-static const char err_msg[] = "could not trace into fd given by "
-   "GIT_TRACE environment variable";
-
 static int prepare_trace_line(const char *file, int line,
  struct trace_key *key, struct strbuf *buf)
 {
@@ -133,8 +130,11 @@ static int prepare_trace_line(const char *file, int line,
 
 static void trace_write(struct trace_key *key, const void *buf, unsigned len)
 {
-   if (write_in_full(get_trace_fd(key), buf, len) < 0)
-   warning("%s: write error (%s)", err_msg, strerror(errno));
+   if (write_in_full(get_trace_fd(key), buf, len) < 0) {
+   normalize_trace_key();
+   warning("unable to write trace for %s: %s",
+   key->key, strerror(errno));
+   }
 }
 
 void trace_verbatim(struct trace_key *key, const void *buf, unsigned len)
-- 
2.9.2.670.g42e63de

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

What's cooking (interim report)

[Junio C Hamano]

Quite a many topics have been merged to 'master' and 'next'.

As I just did a full "What's cooking" yesterday, I'd only list the
new topics and "highlights", the latter of which are what would
benefit from the last round of eyeballs from reviewers.

Many topics have been in 'Cooking' section even though they do not
show any progress; I'll start moving them to 'Stalled' and then
'Discarded' soonish.  Note that "Discarded" does not mean "Rejected,
don't talk about this ever again"; it merely means "With remaining
issue left unaddressed, I do not see a point in keeping it in my
tree right now".

--
[New Topics]

* jh/clean-smudge-f-doc (2016-08-03) 1 commit
 - clarify %f documentation

 Split out from a stalled jh/clean-smudge-annex topic before
 discarding it.

 Will merge to 'next'.


* jh/status-v2-porcelain (2016-08-03) 8 commits
 - status: tests for --porcelain=v2
 - git-status.txt: describe --porcelain=v2 format
 - status: print branch info with --porcelain=v2 --branch
 - status: print per-file porcelain v2 status data
 - status: per-file data collection for --porcelain=v2
 - status: support --porcelain[=]
 - status: cleanup API to wt_status_print
 - status: rename long-format print routines

 Enhance "git status --porcelain" output by collecting more data on
 the state of the index and the working tree files, which may
 further be used to teach git-prompt (in contrib/) to make fewer
 calls to git.


* jk/parseopt-string-list (2016-08-03) 1 commit
 - blame: drop strdup of string literal

 A recent API change to parse_opt_string_list() introduced a small
 unintended memory leak in the command line parsing of "git blame",
 which has been plugged.

 Will merge to 'next'.


* js/import-tars-hardlinks (2016-08-03) 1 commit
 - import-tars: support hard links

 "import-tars" fast-import script (in contrib/) used to ignore a
 hardlink target and replaced it with an empty file, which has been
 corrected to record the same blob as the other file the hardlink is
 shared with.


* js/t4130-rename-without-ino (2016-08-03) 1 commit
 - t4130: work around Windows limitation

 Windows port was failing some tests in t4130, due to the lack of
 inum in the returned values by its lstat(2) emulation.

 Will merge to 'next' and then to 'master' and then to 'maint'.


* nd/fbsd-lazy-mtime (2016-08-03) 2 commits
 - SQUASH???
 - t7063: work around FreeBSD's lazy mtime update feature

 FreeBSD can lie when asked mtime of a directory, which made the
 untracked cache code to fall back to a slow-path, which in turn
 caused tests in t7063 to fail because it wanted to verify the
 behaviour of the fast-path.

 Waiting for a response to SQUASH???


* sb/submodule-recommend-shallowness (2016-08-03) 1 commit
 - gitmodules: document shallow recommendation

 Doc update.

 Will merge to 'next'.

--
[Cooking]

* ew/build-time-pager-tweaks (2016-08-03) 2 commits
 - SQUASH???
 - pager: move pager-specific setup into the build

 The build procedure learned PAGER_ENV knob that lists what default
 environment variable settings to export for popular pagers.  This
 mechanism is used to tweak the default settings to MORE on FreeBSD.

 Expecting a reroll.


* ib/t3700-add-chmod-x-updates (2016-08-01) 3 commits
  (merged to 'next' on 2016-08-03 at 1753346)
 + t3700: add a test_mode_in_index helper function
 + t3700: merge two tests into one
 + t3700: remove unwanted leftover files before running new tests

 The t3700 test about "add --chmod=-x" have been made a bit more
 robust and generally cleaned up.

 Will merge to 'master'.

This may want to go to 'maint'.


* sb/submodule-update-dot-branch (2016-08-01) 7 commits
 - submodule update: allow '.' for branch value
 - submodule--helper: add remote-branch helper
 - submodule-config: keep configured branch around
 - submodule--helper: fix usage string for relative-path
 - submodule update: narrow scope of local variable
 - submodule update: respect depth in subsequent fetches
 - t7406: future proof tests with hard coded depth

 A few updates to "git submodule update".

 Will merge to 'next'.


* jc/hashmap-doc-init (2016-08-02) 1 commit
 - hashmap: clarify that hashmap_entry can safely be discarded

 The API documentation for hashmap was unclear if hashmap_entry
 can be safely discarded without any other consideration.  State
 that it is safe to do so.


* jk/pack-objects-optim-mru (2016-07-29) 1 commit
 - pack-objects: use mru list when iterating over packs
 (this branch uses jk/pack-objects-optim.)

 This is still questionable in that it can attempt to create a cycle
 in delta-chain, only to be stopped by the last-ditch recovery logic
 in there.


* kw/patch-ids-optim (2016-07-29) 4 commits
 - rebase: avoid computing unnecessary patch IDs
 - patch-ids: add flag to create the diff patch id using header only data
 - patch-ids: replace the seen indicator with a commit pointer
 - patch-ids: stop using a 

Re: [PATCH 0/7] minor trace fixes and cosmetic improvements

[Jeff King]

On Wed, Aug 03, 2016 at 06:56:00PM -0400, Jeff King wrote:

> On Wed, Aug 03, 2016 at 05:39:20PM -0400, Jeff King wrote:
> 
> > Thinking about (2), I'd go so far as to say that the trace actually
> > should just be using:
> > 
> >   if (write_in_full(...) < 0)
> > warning("unable to write trace to ...: %s", strerror(errno));
> > 
> > and we should get rid of write_or_whine_pipe entirely.
> 
> I started to write a patch to do that, but it turns out the trace code
> is full of bugs (and opportunities for cosmetic improvements).
> 
> Here's what I came up with.
> 
>   [1/7]: trace: handle NULL argument in trace_disable()
>   [2/7]: trace: stop using write_or_whine_pipe()
>   [3/7]: trace: use warning() for printing trace errors
>   [4/7]: trace: cosmetic fixes for error messages
>   [5/7]: trace: correct variable name in write() error message
>   [6/7]: trace: disable key after write error
>   [7/7]: write_or_die: drop write_or_whine_pipe()

Oops, I meant to detach this from the parent thread, but apparently I am
incompetent at editing email headers.

This really is totally orthogonal to your series (except that you should
obviously not use write_or_whine_pipe(), but that is the case whether I
rip it out or not :) ).

-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/7] trace: handle NULL argument in trace_disable()

[Jeff King]

All of the trace functions treat a NULL key as a synonym for
the default GIT_TRACE key. Except for trace_disable(), which
will segfault.

Fortunately, this can't cause any bugs, as the function has
no callers. But rather than drop it, let's fix the bug, as I
plan to add a caller.

Signed-off-by: Jeff King 
---
 trace.c | 20 
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/trace.c b/trace.c
index 4aeea60..f204a7d 100644
--- a/trace.c
+++ b/trace.c
@@ -25,15 +25,25 @@
 #include "cache.h"
 #include "quote.h"
 
+/*
+ * "Normalize" a key argument by converting NULL to our trace_default,
+ * and otherwise passing through the value. All caller-facing functions
+ * should normalize their inputs in this way, though most get it
+ * for free by calling get_trace_fd() (directly or indirectly).
+ */
+static void normalize_trace_key(struct trace_key **key)
+{
+   static struct trace_key trace_default = { "GIT_TRACE" };
+   if (!*key)
+   *key = _default;
+}
+
 /* Get a trace file descriptor from "key" env variable. */
 static int get_trace_fd(struct trace_key *key)
 {
-   static struct trace_key trace_default = { "GIT_TRACE" };
const char *trace;
 
-   /* use default "GIT_TRACE" if NULL */
-   if (!key)
-   key = _default;
+   normalize_trace_key();
 
/* don't open twice */
if (key->initialized)
@@ -75,6 +85,8 @@ static int get_trace_fd(struct trace_key *key)
 
 void trace_disable(struct trace_key *key)
 {
+   normalize_trace_key();
+
if (key->need_close)
close(key->fd);
key->fd = 0;
-- 
2.9.2.670.g42e63de

--
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 6/7] trace: disable key after write error

[Jeff King]

If we get a write error writing to a trace descriptor, the
error isn't likely to go away if we keep writing. Instead,
you'll just get the same error over and over. E.g., try:

  GIT_TRACE_PACKET=42 git ls-remote >/dev/null

You don't really need to see:

  warning: unable to write trace for GIT_TRACE_PACKET: Bad file descriptor

hundreds of times. We could fallback to tracing to stderr,
as we do in the error code-path for open(), but there's not
much point. If the user fed us a bogus descriptor, they're
probably better off fixing their invocation. And if they
didn't, and we saw a transient error (e.g., ENOSPC writing
to a file), it probably doesn't help anybody to have half of
the trace in a file, and half on stderr.

Signed-off-by: Jeff King 
---
 trace.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/trace.c b/trace.c
index 4efb256..083eb98 100644
--- a/trace.c
+++ b/trace.c
@@ -134,6 +134,7 @@ static void trace_write(struct trace_key *key, const void 
*buf, unsigned len)
normalize_trace_key();
warning("unable to write trace for %s: %s",
key->key, strerror(errno));
+   trace_disable(key);
}
 }
 
-- 
2.9.2.670.g42e63de

--
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 0/7] minor trace fixes and cosmetic improvements

[Jeff King]

On Wed, Aug 03, 2016 at 05:39:20PM -0400, Jeff King wrote:

> Thinking about (2), I'd go so far as to say that the trace actually
> should just be using:
> 
>   if (write_in_full(...) < 0)
>   warning("unable to write trace to ...: %s", strerror(errno));
> 
> and we should get rid of write_or_whine_pipe entirely.

I started to write a patch to do that, but it turns out the trace code
is full of bugs (and opportunities for cosmetic improvements).

Here's what I came up with.

  [1/7]: trace: handle NULL argument in trace_disable()
  [2/7]: trace: stop using write_or_whine_pipe()
  [3/7]: trace: use warning() for printing trace errors
  [4/7]: trace: cosmetic fixes for error messages
  [5/7]: trace: correct variable name in write() error message
  [6/7]: trace: disable key after write error
  [7/7]: write_or_die: drop write_or_whine_pipe()

-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 7/7] write_or_die: drop write_or_whine_pipe()

[Jeff King]

This function has no callers, and is not likely to gain any
because it's confusing to use.

It unconditionally complains to stderr, but _doesn't_ die.
Yet any caller which wants a "gentle" write would generally
want to suppress the error message, because presumably
they're going to write a better one, and/or try the
operation again.

And the check_pipe() call leads to confusing behaviors. It
means we die for EPIPE, but not for other errors, which is
confusing and pointless.

On top of all that, it has unusual error return semantics,
which makes it easy for callers to get it wrong.

Let's drop the function, and if somebody ever needs to
resurrect something like it, they can fix these warts.

Signed-off-by: Jeff King 
---
 cache.h|  1 -
 write_or_die.c | 12 
 2 files changed, 13 deletions(-)

diff --git a/cache.h b/cache.h
index b5f76a4..3885911 100644
--- a/cache.h
+++ b/cache.h
@@ -1740,7 +1740,6 @@ extern int copy_file(const char *dst, const char *src, 
int mode);
 extern int copy_file_with_time(const char *dst, const char *src, int mode);
 
 extern void write_or_die(int fd, const void *buf, size_t count);
-extern int write_or_whine_pipe(int fd, const void *buf, size_t count, const 
char *msg);
 extern void fsync_or_die(int fd, const char *);
 
 extern ssize_t read_in_full(int fd, void *buf, size_t count);
diff --git a/write_or_die.c b/write_or_die.c
index 9816879..0734432 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -82,15 +82,3 @@ void write_or_die(int fd, const void *buf, size_t count)
die_errno("write error");
}
 }
-
-int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg)
-{
-   if (write_in_full(fd, buf, count) < 0) {
-   check_pipe(errno);
-   fprintf(stderr, "%s: write error (%s)\n",
-   msg, strerror(errno));
-   return 0;
-   }
-
-   return 1;
-}
-- 
2.9.2.670.g42e63de
--
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 3/7] trace: use warning() for printing trace errors

[Jeff King]

Right now we just fprintf() straight to stderr, which can
make the output hard to distinguish. It would be helpful to
give it one of our usual prefixes like "error:", "warning:",
etc.

It doesn't make sense to use error() here, as the trace code
is "optional" debugging code. If something goes wrong, we
should warn the user, but saying "error" implies the actual
git operation had a problem. So warning() is the only sane
choice.

Note that this does end up calling warn_routine() to do the
formatting. So in theory, somebody who tries to trace from
their warn_routine() could cause a loop. But nobody does
this, and in fact nobody in the history of git has ever
replaced the default warn_builtin (there isn't even a
set_warn_routine function!).

Signed-off-by: Jeff King 
---
 trace.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/trace.c b/trace.c
index bdbe149..6a77e4d 100644
--- a/trace.c
+++ b/trace.c
@@ -61,9 +61,8 @@ static int get_trace_fd(struct trace_key *key)
else if (is_absolute_path(trace)) {
int fd = open(trace, O_WRONLY | O_APPEND | O_CREAT, 0666);
if (fd == -1) {
-   fprintf(stderr,
-   "Could not open '%s' for tracing: %s\n"
-   "Defaulting to tracing on stderr...\n",
+   warning("Could not open '%s' for tracing: %s\n"
+   "Defaulting to tracing on stderr...",
trace, strerror(errno));
key->fd = STDERR_FILENO;
} else {
@@ -71,10 +70,10 @@ static int get_trace_fd(struct trace_key *key)
key->need_close = 1;
}
} else {
-   fprintf(stderr, "What does '%s' for %s mean?\n"
+   warning("What does '%s' for %s mean?\n"
"If you want to trace into a file, then please set "
"%s to an absolute pathname (starting with /).\n"
-   "Defaulting to tracing on stderr...\n",
+   "Defaulting to tracing on stderr...",
trace, key->key, key->key);
key->fd = STDERR_FILENO;
}
@@ -135,7 +134,7 @@ static int prepare_trace_line(const char *file, int line,
 static void trace_write(struct trace_key *key, const void *buf, unsigned len)
 {
if (write_in_full(get_trace_fd(key), buf, len) < 0)
-   fprintf(stderr, "%s: write error (%s)\n", err_msg, 
strerror(errno));
+   warning("%s: write error (%s)", err_msg, strerror(errno));
 }
 
 void trace_verbatim(struct trace_key *key, const void *buf, unsigned len)
-- 
2.9.2.670.g42e63de

--
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 2/7] trace: stop using write_or_whine_pipe()

[Jeff King]

The write_or_whine_pipe function does two things:

  1. it checks for EPIPE and converts it into a signal death

  2. it prints a message to stderr on error

The first thing does not help us, and actively hurts.
Generally we would simply die from SIGPIPE in this case,
unless somebody has taken the time to ignore SIGPIPE for the
whole process.  And if they _did_ do that, it seems rather
silly for the trace code, which otherwise takes pains to
continue even in the face of errors (e.g., by not using
write_or_die!), to take down the whole process for one
specific type of error.

Nor does the second thing help us; it just makes it harder
to write our error message, because we have to feed bits of
it as an argument to write_or_whine_pipe(). Translators
never get to see the full message, and it's hard for us to
customize it.

Let's switch to just using write_in_full() and writing our
own error string. For now, the error is identical to what
write_or_whine_pipe() would say, but now that it's more
under our control, we can improve it in future patches.

Signed-off-by: Jeff King 
---
 trace.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/trace.c b/trace.c
index f204a7d..bdbe149 100644
--- a/trace.c
+++ b/trace.c
@@ -132,18 +132,23 @@ static int prepare_trace_line(const char *file, int line,
return 1;
 }
 
+static void trace_write(struct trace_key *key, const void *buf, unsigned len)
+{
+   if (write_in_full(get_trace_fd(key), buf, len) < 0)
+   fprintf(stderr, "%s: write error (%s)\n", err_msg, 
strerror(errno));
+}
+
 void trace_verbatim(struct trace_key *key, const void *buf, unsigned len)
 {
if (!trace_want(key))
return;
-   write_or_whine_pipe(get_trace_fd(key), buf, len, err_msg);
+   trace_write(key, buf, len);
 }
 
 static void print_trace_line(struct trace_key *key, struct strbuf *buf)
 {
strbuf_complete_line(buf);
-
-   write_or_whine_pipe(get_trace_fd(key), buf->buf, buf->len, err_msg);
+   trace_write(key, buf->buf, buf->len);
strbuf_release(buf);
 }
 
-- 
2.9.2.670.g42e63de

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

Re: [PATCH v4 07/12] run-command: add clean_on_exit_handler

[Jeff King]

On Thu, Aug 04, 2016 at 12:15:46AM +0200, Lars Schneider wrote:

> > I'm not clear on why we want this cleanup filter. It looks like you use
> > it in the final patch to send an explicit shutdown to any filters we
> > start. But I see two issues with that:
> > 
> >  1. This shutdown may come at any time, and you have no idea what state
> > the protocol conversation with the filter is in. You could be in
> > the middle of sending another pkt-line, or in a sequence of non-command
> > pkt-lines where "shutdown" is not recognized.
> 
> Maybe I am missing something, but I don't think that can happen because 
> the cleanup callback is *only* executed if Git exits normally without error. 
> In that case we would be in a sane protocol state, no?

OK, then maybe I am doubly missing the point. I thought this cleanup was
here to hit the case where we call die() and git exits unexpectedly.

If you only want to cover the "we are done, no errors, goodbye" case,
then why don't you just write shutdown when we're done?

I realize you may have multiple filters, but I don't think it should be
run-command's job to iterate over them. You are presumably keeping a
list of active filters, and should have a function to iterate over that.

Or better yet, do not require a shutdown at all. The filter sees EOF and
knows there is nothing more to do. If we are in the middle of an
operation, then it knows git died. If not, then presumably git had
nothing else to say (and really, it is not the filter's business if git
saw an error or not).

Though...

> Thanks. The shutdown command is not intended to be a mechanism to tell
> the filter that everything went well. At this point - as you mentioned -
> the filter already received all data in the right way. The shutdown
> command is intended to give the filter some time to perform some post
> processing before Git returns.
> 
> See here for some brainstorming how this feature could be useful
> in filters similar to Git LFS:
> https://github.com/github/git-lfs/issues/1401#issuecomment-236133991

OK, so it is not really "tell the filter to shutdown" but "I am done
with you, filter, but I will wait for you to tell me you are all done,
so that I can tell the user".

I'm not sure if calling that "shutdown" makes sense, though. It's almost
more of a checkpoint (and I wonder if git would ever want to
"checkpoint" without hanging up the connection).

-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 8/8] diff: improve positioning of add/delete blocks in diffs

[Stefan Beller]

On Wed, Aug 3, 2016 at 3:41 PM, Michael Haggerty  wrote:
> On 08/04/2016 12:30 AM, Stefan Beller wrote:
>> On Wed, Aug 3, 2016 at 3:00 PM, Michael Haggerty  
>> wrote:
>>
>>> +return 10 * score - bonus;
>>
>> Would it make sense to define-ify the 10 as well
>> as this is the only hardcoded number in the
>> scoring function?
>
> I started answering this question by explaining why it is not important
> to *optimize* the number 10 (namely because scores are only ever
> compared against other scores, so an overall scaling factor makes no
> difference). The factor 10 only has to be large enough to provide enough
> dynamic range for the other (adjustable) parameters.

But it only scales the score, not the bonus. So another way to write it
would be

score - bonus/10;

assuming the values of score and bonus are large enough.

In some prior conversation you said you take the indent and add an epsilon
for some special conditions to make one indent better than the other.

Epsilons are usually very small compared to the rest of the equation,
but if I look at the boni definitions ranging from -63..50 they are scaled up
so much that the bonus may become larger than '1' unit of 'score', i.e.
it is not an epsilon any more. Or to put it another way:
If we were to s/10/100/ the results would be worse.

Rather the 10 describes the ratio of "advanced magic" to pure indentation
based scoring in my understanding.

>
> But I think you are asking a simpler question: should we give this
> constant a name rather than hardcoding it? I don't see a strong reason
> for or against, so I'll give it a name in the next version, as you suggest.

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

[Jakub Narębski]

[Note that some of this might have been invalidated by v4]

W dniu 01.08.2016 o 15:32, Lars Schneider pisze:
>> On 31 Jul 2016, at 00:05, Jakub Narębski  wrote:
>> W dniu 30.07.2016 o 01:38, larsxschnei...@gmail.com pisze:

>>> 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
 
With current implementation, Git checks second packet of the handshake
for version, and third packet for capabilities.  The "capabilities" or
"capabilities=" is entirely redundant; it is the position of packet
(it is the packet number) that matters.  At least for now.

The only thing that "version" in "version 2" and "capabilities"
in "capabilities: clean smudge" helps is self-describing of the protocol.

To really make use of them you would have to end handshake with flush
packet, and do a parsing: loop over every packet, and match known
patterns.  Well, perhaps with exception of known header: it doesn't
makes sense to have "version N" anywhere else than second packet,
and it doesn't makes sense to repeat it.

We also don't want to proliferate packets unnecessarily.  Each packet
is a bit (a tiny bit) of a performance hit.
 
>>> 
>>> 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.

We could, following the example of original per-file filter drivers.
It is as simple as starting writer using start_async(), as if we did
writing from Git in a child process.

Though that might be left for later (assuming that protocol is flexible
enough), as synchronous protocol (write, then read) is a bit simpler to
implement.

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

Providing size upfront can be a hint for filter or Git.  For example
HTTP provides Content-Length: header, though it is not strictly necessary.

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

The solution with stopping writing wouldn't work, I don't think.

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

Right, better to design and implement basic protocol, taking care that
it is extensible, and only then add to it.

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

All right.

>>> After the filter has processed a blob it is expected to wait for
>>> the next command. A demo implementation can be found in
>>> `t/t0021/rot13-filter.pl` located in the Git core repository.
>>
>> If filter does not support "shutdown" capability (or if said
>> capability is postponed for later patch), it should behave sanely
>> when Git command reaps it (SIGTERM + wait + SIGKILL?, SIGCHLD?).
> 
> How would you do this? Don't you think the current solution is
> good enough for processes that don't need a proper shutdown?

Actually... couldn't filter driver register atexit() / signal handler
to do a clean exit, if it is needed?
 
 
>> I wonder if it would be worth it to explain the reasoning behind
>> this solution and show alternate ones.
>>
>> * Using a separate variable to signal that filters are invoked
>>   per-command rather than per-file, and use pkt-line interface,
>>   like boolean-valued `useProtocol`, 

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

[Michael Haggerty]

On 08/04/2016 12:30 AM, Stefan Beller wrote:
> On Wed, Aug 3, 2016 at 3:00 PM, Michael Haggerty  wrote:
> 
>> +return 10 * score - bonus;
> 
> Would it make sense to define-ify the 10 as well
> as this is the only hardcoded number in the
> scoring function?

I started answering this question by explaining why it is not important
to *optimize* the number 10 (namely because scores are only ever
compared against other scores, so an overall scaling factor makes no
difference). The factor 10 only has to be large enough to provide enough
dynamic range for the other (adjustable) parameters.

But I think you are asking a simpler question: should we give this
constant a name rather than hardcoding it? I don't see a strong reason
for or against, so I'll give it a name in the next version, as you suggest.

Michael

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

Re: [PATCH v4 11/12] convert: add filter..process option

[Jeff King]

On Wed, Aug 03, 2016 at 11:48:00PM +0200, Lars Schneider wrote:

> OK. Is this the v2 discussion you are referring to?
> http://public-inbox.org/git/1461972887-22100-1-git-send-email-sbeller%40google.com/
> 
> What format do you suggest?
> 
> packet:  git< git-filter-protocol\n
> packet:  git< version=2\n
> packet:  git< capability=clean\n
> packet:  git< capability=smudge\n
> packet:  git< 
> 
> or
> 
> packet:  git< git-filter-protocol\n
> packet:  git< version=2\n
> packet:  git< capability\n
> packet:  git< clean\n
> packet:  git< smudge\n
> packet:  git< 
> 
> or  ... ?
> 
> I would prefer the first one, I think.

How about:

  version=2
  clean=true
  smudge=true
  

? Then we do not have to care about multiple "capability" keys (so
something naively parsing this could just store them in a string list,
for example).

You could also make "clean" a synonym for "clean=true" or something, and
have:

  version=2
  clean
  smudge
  

but it's probably better to have the protocol err on the side of
verbose-but-unambiguous. It's not like people are typing this routinely.

-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 8/8] diff: improve positioning of add/delete blocks in diffs

[Michael Haggerty]

On 08/04/2016 12:29 AM, Jacob Keller wrote:
> On Wed, Aug 3, 2016 at 3:00 PM, Michael Haggerty  wrote:
>> +/*
>> + * If a line is indented more than this, get_indent() just returns this 
>> value.
>> + * This avoids having to do absurd amounts of work for data that are not
>> + * human-readable text, and also ensures that the output of get_indent fits 
>> within
>> + * an int.
>> + */
>> +#define MAX_INDENT 200
>> +
>> +/*
>> + * Return the amount of indentation of the specified line, treating TAB as 8
>> + * columns. Return -1 if line is empty or contains only whitespace. Clamp 
>> the
>> + * output value at MAX_INDENT.
>> + */
>> +static int get_indent(xrecord_t *rec)
>> +{
>> +   long i;
>> +   int ret = 0;
>> +
>> +   for (i = 0; i < rec->size; i++) {
>> +   char c = rec->ptr[i];
>> +
>> +   if (!XDL_ISSPACE(c))
>> +   return ret;
>> +   else if (c == ' ')
>> +   ret += 1;
>> +   else if (c == '\t')
>> +   ret += 8 - ret % 8;
>> +   /* ignore other whitespace characters */
>> +
>> +   if (ret >= MAX_INDENT)
>> +   return MAX_INDENT;
> 
> Should we return -1 here?
> 
>> +   }
>> +   /*
>> +* We have reached the end of the line without finding any non-space
>> +* characters; i.e., the whole line consists of trailing spaces, 
>> which we
>> +* are not interested in.
>> +*/
>> +   return -1;
> 
> It seems odd to be that a line with "199" spaces and nothing else will
> return "-1" but a line with 200 spaces and nothing else will return
> 200..? Would it be safe to just return -1 in both cases (if a line is
> all spaces or starts with more than 200 spaces just return -1)?
> 
>> +}
>> +

Thanks for your feedback.

I was implicitly assuming that such lines would have text somewhere
after those 200 spaces (or 25 TABs or whatever). But you're right, the
line could consist only of whitespace. Unfortunately, the only way to
distinguish these two cases is to read the rest of the line, which is
exactly what we *don't* want to do.

But I think it doesn't matter anyway. Such "text" will likely never be
read by a human, so it's not a big deal if the slider position is not
picked perfectly. And remember, this whole saga is just to improve the
aesthetics of the diff. The diff is *correct* (e.g., in the sense of
applicable) regardless of where we position the sliders.

Michael

--
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 8/8] diff: improve positioning of add/delete blocks in diffs

[Stefan Beller]

On Wed, Aug 3, 2016 at 3:00 PM, Michael Haggerty  wrote:

> +return 10 * score - bonus;

Would it make sense to define-ify the 10 as well
as this is the only hardcoded number in the
scoring function?
--
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 8/8] diff: improve positioning of add/delete blocks in diffs

[Jacob Keller]

On Wed, Aug 3, 2016 at 3:00 PM, Michael Haggerty  wrote:
> +/*
> + * If a line is indented more than this, get_indent() just returns this 
> value.
> + * This avoids having to do absurd amounts of work for data that are not
> + * human-readable text, and also ensures that the output of get_indent fits 
> within
> + * an int.
> + */
> +#define MAX_INDENT 200
> +
> +/*
> + * Return the amount of indentation of the specified line, treating TAB as 8
> + * columns. Return -1 if line is empty or contains only whitespace. Clamp the
> + * output value at MAX_INDENT.
> + */
> +static int get_indent(xrecord_t *rec)
> +{
> +   long i;
> +   int ret = 0;
> +
> +   for (i = 0; i < rec->size; i++) {
> +   char c = rec->ptr[i];
> +
> +   if (!XDL_ISSPACE(c))
> +   return ret;
> +   else if (c == ' ')
> +   ret += 1;
> +   else if (c == '\t')
> +   ret += 8 - ret % 8;
> +   /* ignore other whitespace characters */
> +
> +   if (ret >= MAX_INDENT)
> +   return MAX_INDENT;

Should we return -1 here?

> +   }
> +   /*
> +* We have reached the end of the line without finding any non-space
> +* characters; i.e., the whole line consists of trailing spaces, 
> which we
> +* are not interested in.
> +*/
> +   return -1;

It seems odd to be that a line with "199" spaces and nothing else will
return "-1" but a line with 200 spaces and nothing else will return
200..? Would it be safe to just return -1 in both cases (if a line is
all spaces or starts with more than 200 spaces just 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: [RFC/PATCH v11 04/13] bisect--helper: `bisect_clean_state` shell function in C

[Pranit Bauva]

Hey Junio,

On Tue, Aug 2, 2016 at 11:16 PM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>> +static int bisect_clean_state(void)
>> +{
>> + int result = 0;
>> +
>> + /* There may be some refs packed during bisection */
>> + struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
>> + for_each_ref_in("refs/bisect/", mark_for_removal, (void *) 
>> _for_removal);
>> + string_list_append(_for_removal, xstrdup("BISECT_HEAD"));
>> + result = delete_refs(_for_removal);
>> + refs_for_removal.strdup_strings = 1;
>> + string_list_clear(_for_removal, 0);
>> + remove_path(git_path_bisect_expected_rev());
>> + remove_path(git_path_bisect_ancestors_ok());
>> + remove_path(git_path_bisect_log());
>> + remove_path(git_path_bisect_names());
>> + remove_path(git_path_bisect_run());
>> + remove_path(git_path_bisect_terms());
>> + /* Cleanup head-name if it got left by an old version of git-bisect */
>> + remove_path(git_path_head_name());
>> +  * Cleanup BISECT_START last to support the --no-checkout option
>> +  * introduced in the commit 4796e823a.
>> +  */
>> + remove_path(git_path_bisect_start());
>
> I can see that refs/files-backend.c misuses it already, but
> remove_path() helper is about removing a path in the working tree,
> together with any parent directory that becomes empty due to the
> removal.  You do not expect $GIT_DIR/ to become an empty directory
> after removing $GIT_DIR/BISECT_LOG nor want to rmdir $GIT_DIR even
> if it becomes empty.  It is a wrong helper function to use here.
>
> Also you do not seem to check the error from the function to smudge
> the "result" you are returning from this function.

Yes I should combine the results from every removal.

> Isn't unlink_or_warn() more correct helper to use here?

The shell code uses rm -f which is silent and it removes only if
present. So it makes me wonder which would be more appropriate
unlink_or_warn() or remove_or_warn() or remove_path(). Is
remove_path() different from its shell equivalent "rm -f"?

>> + return result;
>> +}

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

Re: [PATCH v4 07/12] run-command: add clean_on_exit_handler

[Lars Schneider]

> On 03 Aug 2016, at 23:24, Jeff King  wrote:
> 
> On Wed, Aug 03, 2016 at 06:42:20PM +0200, larsxschnei...@gmail.com wrote:
> 
>> From: Lars Schneider 
>> 
>> Some commands might need to perform cleanup tasks on exit. Let's give
>> them an interface for doing this.
>> 
>> Please note, that the cleanup callback is not executed if Git dies of a
>> signal. The reason is that only "async-signal-safe" functions would be
>> allowed to be call in that case. Since we cannot control what functions
>> the callback will use, we will not support the case. See 507d7804 for
>> more details.
> 
> I'm not clear on why we want this cleanup filter. It looks like you use
> it in the final patch to send an explicit shutdown to any filters we
> start. But I see two issues with that:
> 
>  1. This shutdown may come at any time, and you have no idea what state
> the protocol conversation with the filter is in. You could be in
> the middle of sending another pkt-line, or in a sequence of non-command
> pkt-lines where "shutdown" is not recognized.

Maybe I am missing something, but I don't think that can happen because 
the cleanup callback is *only* executed if Git exits normally without error. 
In that case we would be in a sane protocol state, no?


>  2. If your protocol does bad things when it is cut off in the middle
> without an explicit shutdown, then it's a bad protocol. As you
> note, this patch doesn't cover signal death, nor could it ever
> cover something like "kill -9", or a bug which prevented git from
> saying "shutdown".
> 
> You're much better off to design the protocol so that a premature
> EOF is detected as an error.  For example, if we're feeding file
> data to the filter, and we're worried it might be writing it to
> a data store (like LFS), we would not want it to see EOF and say
> "well, I guess I got all the data; time to store this!". Instead,
> it should know how many bytes are coming, or should have some kind
> of framing so that the sender says "and now you have seen all the
> bytes" (like a pkt-line flush).
> 
> AFAIK, your protocol _does_ do those things sensibly, so this
> explicit shutdown isn't really accomplishing anything.

Thanks. The shutdown command is not intended to be a mechanism to tell
the filter that everything went well. At this point - as you mentioned -
the filter already received all data in the right way. The shutdown
command is intended to give the filter some time to perform some post
processing before Git returns.

See here for some brainstorming how this feature could be useful
in filters similar to Git LFS:
https://github.com/github/git-lfs/issues/1401#issuecomment-236133991

- 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 2/8] xdl_change_compact(): clarify code

[Stefan Beller]

On Wed, Aug 3, 2016 at 3:00 PM, Michael Haggerty  wrote:
> Write things out a bit longer but less cryptically. Add more comments.

By less cryptic you mean in Git coding style ;)
The original author (do we want to cc Davido?) may disagree.

> +
> +   /*
> +* Are there any blank lines that could appear as the 
> last
> +* line of this group?
> +*/

IIRC this comment is not quite correct as this 'only' counts the number of
blank lines within the forward shifting section, i.e. in the movable space.

Later we use it as a boolean indicator (whether or not it is equal to 0)
to see if we can do better.

Any other change in code and comments looks good to me, but this stood out
like a sore thumb. (Probably the old heuristic as a whole stood out, but the
comment here specifically sounds /wrong/ to me in this place. How can
a question document a variable? I'd rather expect a question comment
to ease the understanding of a condition)

Thanks for reviving this topic!
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

[PATCH 3/8] xdl_change_compact(): rename i to end

[Michael Haggerty]

Rename i to end, and alternate between using start and end as the
indexing variable as appropriate.

Rename ixref to end_matching_other.

Add some more comments.

Signed-off-by: Michael Haggerty 
---
 xdiff/xdiffi.c | 70 --
 1 file changed, 39 insertions(+), 31 deletions(-)

diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index a0a485c..0f235bc 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -414,7 +414,7 @@ static int recs_match(xrecord_t **recs, long ixs, long ix, 
long flags)
 }
 
 int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
-   long i, io, start, ixref, groupsize, nrec = xdf->nrec;
+   long start, end, io, end_matching_other, groupsize, nrec = xdf->nrec;
char *rchg = xdf->rchg, *rchgo = xdfo->rchg;
unsigned int blank_lines;
xrecord_t **recs = xdf->recs;
@@ -424,7 +424,8 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long 
flags) {
 * change groups for a consistent and pretty diff output. This also
 * helps in finding joinable change groups and reduce the diff size.
 */
-   for (i = io = 0;;) {
+   end = io = 0;
+   while (1) {
/*
 * Find the first changed line in the to-be-compacted file.
 * We need to keep track of both indexes, so if we find a
@@ -434,7 +435,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long 
flags) {
 * not need index bounding since the array is prepared with
 * a zero at position -1 and N.
 */
-   for (; i < nrec && !rchg[i]; i++) {
+   for (start = end; start < nrec && !rchg[start]; start++) {
/* skip over any changed lines in the other file... */
while (rchgo[io])
io++;
@@ -442,24 +443,29 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, 
long flags) {
/* ...plus one non-changed line. */
io++;
}
-   if (i == nrec)
+   if (start == nrec)
break;
 
/*
-* Record the start of a changed-group in the to-be-compacted 
file
-* and find the end of it, on both to-be-compacted and other 
file
-* indexes (i and io).
+* That's the start of a changed-group in the to-be-compacted
+* file. Now find its end.
 */
-   start = i++;
-
-   while (rchg[i])
-   i++;
+   end = start + 1;
+   while (rchg[end])
+   end++;
 
while (rchgo[io])
   io++;
 
+   /*
+* Now shift the change up and then down as far as possible in
+* each direction. If it bumps into any other changes, merge 
them.
+* If there are any changes in the other file that this change
+* could line up with, set end_matching_other to the end 
position
+* of this change that would leave them aligned.
+*/
do {
-   groupsize = i - start;
+   groupsize = end - start;
 
/*
 * Are there any blank lines that could appear as the 
last
@@ -472,9 +478,9 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long 
flags) {
 * to the last line of the current change group, shift 
the
 * group backward.
 */
-   while (start > 0 && recs_match(recs, start - 1, i - 1, 
flags)) {
+   while (start > 0 && recs_match(recs, start - 1, end - 
1, flags)) {
rchg[--start] = 1;
-   rchg[--i] = 0;
+   rchg[--end] = 0;
 
/*
 * This change might have joined two change 
groups.
@@ -501,13 +507,13 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, 
long flags) {
 * the other file. Record the end-of-group
 * position:
 */
-   ixref = i;
+   end_matching_other = end;
} else {
/*
 * Otherwise, set a value to signify that there
 * are no matched changes in the other file:
 */
-   ixref = nrec;
+   end_matching_other = -1;
}
 
/*
@@ -515,11 +521,11 @@ int 

[PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

[Michael Haggerty]

Some groups of added/deleted lines in diffs can be slid up or down,
because lines at the edges of the group are not unique. Picking good
shifts for such groups is not a matter of correctness but definitely has
a big effect on aesthetics. For example, consider the following two
diffs. The first is what standard Git emits:

--- a/9c572b21dd090a1e5c5bb397053bf8043ffe7fb4:git-send-email.perl
+++ b/6dcfa306f2b67b733a7eb2d7ded1bc9987809edb:git-send-email.perl
@@ -231,6 +231,9 @@ if (!defined $initial_reply_to && $prompting) {
 }

 if (!$smtp_server) {
+   $smtp_server = $repo->config('sendemail.smtpserver');
+}
+if (!$smtp_server) {
foreach (qw( /usr/sbin/sendmail /usr/lib/sendmail )) {
if (-x $_) {
$smtp_server = $_;

The following diff is equivalent, but is obviously preferable from an
aesthetic point of view:

--- a/9c572b21dd090a1e5c5bb397053bf8043ffe7fb4:git-send-email.perl
+++ b/6dcfa306f2b67b733a7eb2d7ded1bc9987809edb:git-send-email.perl
@@ -230,6 +230,9 @@ if (!defined $initial_reply_to && $prompting) {
$initial_reply_to =~ s/(^\s+|\s+$)//g;
 }

+if (!$smtp_server) {
+   $smtp_server = $repo->config('sendemail.smtpserver');
+}
 if (!$smtp_server) {
foreach (qw( /usr/sbin/sendmail /usr/lib/sendmail )) {
if (-x $_) {

This patch teaches Git to pick better positions for such "diff sliders".

The original Git code basically always shifted such "sliders" as far
down in the file as possible. The only exception is when the slider can
be aligned with a group of the lines in the other file, in which case
Git favors one add+delete block over one add and a slightly offset
delete block. This naive algorithm often yields ugly diffs.

Commit d634d61ed6 improved the situation somewhat by preferring to
position add/delete groups to make their last line a blank line, when
that is possible. This heuristic does more good than harm, but can only
help if there are blank lines in the right places. It still leaves a lot
of ugly diffs.

This commit implements a new and much better heuristic for picking
optimal "slider" positions using the following approach: First observe
that each hypothetical positioning of a diff slider introduces two
splits: one between the context lines preceding the group and the first
added/deleted line, and the other between the last added/deleted line
and the first line of context following it. It tries to find the
positioning that creates the least bad splits.

Splits are evaluated based only on the presence and locations of nearby
blank lines, and the indentation of lines near the split. Basically, it
prefers to introduce splits between lines that are indented less and
adjacent to blank lines. In more detail:

1. It measures the following characteristics of a proposed splitting
   position:

   * the number of blank lines above the proposed split
   * whether the line directly after the split is blank
   * the number of blank lines following that line
   * the indentation of the nearest non-blank line above the split
   * the indentation of the line directly below the split
   * the indentation of the nearest non-blank line after that line

2. It combines these attributes using a bunch of empirically-optimized
   weighting factors to estimate a score of the "badness" of splitting
   the text at that position.

3. It defines the score for a positioning of a diff slider to be the sum
   of the scores for the splits at the top and bottom of the slider.

4. It computes scores like this for all possible positions of the diff
   slider, and selects the position with the smallest "badness" score.

The weighting factors were found by collecting a corpus of code samples
in various programming languages, deciding "by eye" the best positioning
for about 2700 diff sliders, then optimizing the weights against this
corpus to get the best agreement with the manually-determined values.
(One caveat is that the same corpus was used for both optimization and
testing.)

The resulting numbers of non-optimal diff groups were as follows:

| repository   | default | compaction | indent | optimized |
|  | --- | -- | -- | - |
| ant  | 225 |102 |  7 | 5 |
| bugzilla | 208 | 81 | 14 | 8 |
| couchdb  |  44 | 24 | 13 | 4 |
| docker   | 180 |160 | 29 | 7 |
| git  | 446 | 59 | 27 | 4 |
| ipython  | 358 |163 | 61 |11 |
| junit| 146 | 67 |  5 | 1 |
| nodejs   | 489 | 78 | 12 | 2 |
| phpmyadmin   | 330 | 49 |  1 | 0 |
| test-more|  15 |  2 |  2 | 0 

[PATCH 0/8] Better heuristics make prettier diffs

[Michael Haggerty]

I've talked about this quite a bit on the list already. The idea is to
improve ugly diffs like

@@ -231,6 +231,9 @@ if (!defined $initial_reply_to && $prompting) {
 }

 if (!$smtp_server) {
+   $smtp_server = $repo->config('sendemail.smtpserver');
+}
+if (!$smtp_server) {
foreach (qw( /usr/sbin/sendmail /usr/lib/sendmail )) {
if (-x $_) {
$smtp_server = $_;

by feeding clues from the surrounding lines (namely their patterns of
indentation and blank lines) into a heuristic that more often produces
the diffs that users would rather see, like

--- a/9c572b21dd090a1e5c5bb397053bf8043ffe7fb4:git-send-email.perl
+++ b/6dcfa306f2b67b733a7eb2d7ded1bc9987809edb:git-send-email.perl
@@ -230,6 +230,9 @@ if (!defined $initial_reply_to && $prompting) {
$initial_reply_to =~ s/(^\s+|\s+$)//g;
 }

+if (!$smtp_server) {
+   $smtp_server = $repo->config('sendemail.smtpserver');
+}
 if (!$smtp_server) {
foreach (qw( /usr/sbin/sendmail /usr/lib/sendmail )) {
if (-x $_) {

See the last commit's log message for a very detailed explanation of
the heuristic, how it was optimized, and how you can get involved to
make sure that the heuristic also works well for your favorite
language.

When tested against a corpus of 2700 diffs that I optimized by hand,
this heuristic gets a "wrong" answer only about 1.7% as frequently as
the current default Git algorithm and only about 5.3% as often as `git
diff --compaction-heuristic`. (Though please don't treat these numbers
as final; I want to verify them again first.)

For now the new algorithm has to be enabled explicitly using either
`--indent-heuristic` or `git config diff.indentheuristic true`.

Michael Haggerty (8):
  xdl_change_compact(): rename some local variables for clarity
  xdl_change_compact(): clarify code
  xdl_change_compact(): rename i to end
  xdl_change_compact(): do one final shift or the other, not both
  xdl_change_compact(): fix compaction heuristic to adjust io
  xdl_change_compact(): keep track of the earliest end
  is_blank_line: take a single xrecord_t as argument
  diff: improve positioning of add/delete blocks in diffs

 Documentation/diff-options.txt |   6 +-
 diff.c |  11 +
 git-add--interactive.perl  |   5 +-
 xdiff/xdiff.h  |   1 +
 xdiff/xdiffi.c | 458 ++---
 5 files changed, 409 insertions(+), 72 deletions(-)

-- 
2.8.1

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

[PATCH 1/8] xdl_change_compact(): rename some local variables for clarity

[Michael Haggerty]

* ix -> i
* ixo -> io
* ixs -> start
* grpsiz -> groupsize

Signed-off-by: Michael Haggerty 
---
Thankfully, we don't have to limit indentifers to six characters, so
start naming things more understandably.

 xdiff/xdiffi.c | 66 +-
 1 file changed, 33 insertions(+), 33 deletions(-)

diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index b3c6848..ff7fc42 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -414,7 +414,7 @@ static int recs_match(xrecord_t **recs, long ixs, long ix, 
long flags)
 }
 
 int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
-   long ix, ixo, ixs, ixref, grpsiz, nrec = xdf->nrec;
+   long i, io, start, ixref, groupsize, nrec = xdf->nrec;
char *rchg = xdf->rchg, *rchgo = xdfo->rchg;
unsigned int blank_lines;
xrecord_t **recs = xdf->recs;
@@ -424,7 +424,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long 
flags) {
 * change groups for a consistent and pretty diff output. This also
 * helps in finding joinable change groups and reduce the diff size.
 */
-   for (ix = ixo = 0;;) {
+   for (i = io = 0;;) {
/*
 * Find the first changed line in the to-be-compacted file.
 * We need to keep track of both indexes, so if we find a
@@ -434,22 +434,22 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, 
long flags) {
 * not need index bounding since the array is prepared with
 * a zero at position -1 and N.
 */
-   for (; ix < nrec && !rchg[ix]; ix++)
-   while (rchgo[ixo++]);
-   if (ix == nrec)
+   for (; i < nrec && !rchg[i]; i++)
+   while (rchgo[io++]);
+   if (i == nrec)
break;
 
/*
 * Record the start of a changed-group in the to-be-compacted 
file
 * and find the end of it, on both to-be-compacted and other 
file
-* indexes (ix and ixo).
+* indexes (i and io).
 */
-   ixs = ix;
-   for (ix++; rchg[ix]; ix++);
-   for (; rchgo[ixo]; ixo++);
+   start = i;
+   for (i++; rchg[i]; i++);
+   for (; rchgo[io]; io++);
 
do {
-   grpsiz = ix - ixs;
+   groupsize = i - start;
blank_lines = 0;
 
/*
@@ -457,9 +457,9 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long 
flags) {
 * the last line of the current change group, shift 
backward
 * the group.
 */
-   while (ixs > 0 && recs_match(recs, ixs - 1, ix - 1, 
flags)) {
-   rchg[--ixs] = 1;
-   rchg[--ix] = 0;
+   while (start > 0 && recs_match(recs, start - 1, i - 1, 
flags)) {
+   rchg[--start] = 1;
+   rchg[--i] = 0;
 
/*
 * This change might have joined two change 
groups,
@@ -467,8 +467,8 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long 
flags) {
 * the start index accordingly (and so the 
other-file
 * end-of-group index).
 */
-   for (; rchg[ixs - 1]; ixs--);
-   while (rchgo[--ixo]);
+   for (; rchg[start - 1]; start--);
+   while (rchgo[--io]);
}
 
/*
@@ -477,18 +477,18 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, 
long flags) {
 * change record before the end-of-group index in the 
other
 * file is set).
 */
-   ixref = rchgo[ixo - 1] ? ix: nrec;
+   ixref = rchgo[io - 1] ? i : nrec;
 
/*
 * If the first line of the current change group, is 
equal to
 * the line next of the current change group, shift 
forward
 * the group.
 */
-   while (ix < nrec && recs_match(recs, ixs, ix, flags)) {
-   blank_lines += is_blank_line(recs, ix, flags);
+   while (i < nrec && recs_match(recs, start, i, flags)) {
+   blank_lines += is_blank_line(recs, i, flags);
 
-   rchg[ixs++] = 0;
-   rchg[ix++] = 1;
+   rchg[start++] = 0;
+ 

Re: [PATCH 0/8] Better heuristics make prettier diffs

[Michael Haggerty]

On 08/04/2016 12:00 AM, Michael Haggerty wrote:
> I've talked about this quite a bit on the list already. The idea is to
> improve ugly diffs

I forgot to note that this patch series is also available from my GitHub
account [1] as branch "diff-indent-heuristics".

Also, I will be away from my computer for the next five days, so don't
be insulted if I don't respond promptly to your emails. (Well, actually
I'm never that prompt so people might not even notice :-/ ).

Michael

[1] https://github.com/mhagger/git

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

Re: [PATCH v4 11/12] convert: add filter..process option

[Lars Schneider]

> On 03 Aug 2016, at 23:43, Junio C Hamano  wrote:
> 
> On Wed, Aug 3, 2016 at 2:37 PM, Lars Schneider  
> wrote:
>>> 
>>> I think this was already pointed out in the previous review by Peff,
>>> but a variable "ret" that says "0 is bad" somehow makes it hard to
>>> follow the code.  Perhaps rename it to "int error", flip the meaning,
>>> and if the caller wants this function to return non-zero on success
>>> flip the polarity in the return statement itself, i.e. "return !errors",
>>> may make it easier to follow?
>> 
>> This follows the existing filter function. Please see Peff's later
>> reply here:
> 
> Which I did before mentioning "pointed out in his review".
> 
>> That's why I kept it the way it is. If you prefer the "!errors" approach
>> then I will change that.
> 
> I am not suggesting to change the RETURN VALUE from this function.
> That is why I mentioned "return !errors" to flip the polarity at the end.
> Inside the function, "ret" variable _forces_ the readers to think "this
> function unlike the others signal an error with 0" constantly while
> reading it, and one possible approach to reduce the mental burden
> is to replace "ret" variable with "errors" variable, which is clear to
> anybody that it would be non-zero when we saw error(s).
> 
> Oh, I am not suggesting to _count_ the number of errors by
> mentioning a possible variable name "errors"; the only reason
> why I mentioned that name is because "error" is already
> taken, and "seen_error" is a bit too long.

Agreed. I got that you didn't suggest to change the return value :-)
In order to be consistent I would also adjust the error handling in
the existing apply_filter() function that I renamed to 
apply_single_file_filter() in 11/12. OK?

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

Re: [PATCH v4 11/12] convert: add filter..process option

[Junio C Hamano]

On Wed, Aug 3, 2016 at 2:37 PM, Lars Schneider  wrote:
>>
>> I think this was already pointed out in the previous review by Peff,
>> but a variable "ret" that says "0 is bad" somehow makes it hard to
>> follow the code.  Perhaps rename it to "int error", flip the meaning,
>> and if the caller wants this function to return non-zero on success
>> flip the polarity in the return statement itself, i.e. "return !errors",
>> may make it easier to follow?
>
> This follows the existing filter function. Please see Peff's later
> reply here:

Which I did before mentioning "pointed out in his review".

> That's why I kept it the way it is. If you prefer the "!errors" approach
> then I will change that.

I am not suggesting to change the RETURN VALUE from this function.
That is why I mentioned "return !errors" to flip the polarity at the end.
Inside the function, "ret" variable _forces_ the readers to think "this
function unlike the others signal an error with 0" constantly while
reading it, and one possible approach to reduce the mental burden
is to replace "ret" variable with "errors" variable, which is clear to
anybody that it would be non-zero when we saw error(s).

Oh, I am not suggesting to _count_ the number of errors by
mentioning a possible variable name "errors"; the only reason
why I mentioned that name is because "error" is already
taken, and "seen_error" is a bit too long.
--
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 4/8] xdl_change_compact(): do one final shift or the other, not both

[Michael Haggerty]

There is no need to shift the group to match a diff in the other file if
we're just going to override that shift based on the compaction
heuristic. Note that this changes the behavior if the matching shift
would have shifted the group higher than the last blank line: the old
code would have ignored the compaction heuristic in that case, whereas
the new code always gives precedence to the compaction heuristic when it
is turned on.

Signed-off-by: Michael Haggerty 
---
 xdiff/xdiffi.c | 44 ++--
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 0f235bc..c67cfe3 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -547,11 +547,28 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, 
long flags) {
}
} while (groupsize != end - start);
 
-   /*
-* Try to move back the possibly merged group of changes, to 
match
-* the recorded position in the other file.
-*/
-   if (end_matching_other != -1) {
+   if ((flags & XDF_COMPACTION_HEURISTIC) && blank_lines) {
+   /*
+* Compaction heuristic: if a group can be moved back 
and
+* forth, then if possible shift the group to make its
+* bottom line a blank line.
+*
+* As we already shifted the group forward as far as
+* possible in the earlier loop, we only need to handle
+* backward shifts, not forward ones.
+*/
+   while (start > 0 &&
+  !is_blank_line(recs, end - 1, flags) &&
+  recs_match(recs, start - 1, end - 1, flags)) {
+   rchg[--start] = 1;
+   rchg[--end] = 0;
+   }
+   } else if (end_matching_other != -1) {
+   /*
+* Move the possibly merged group of changes back to 
line
+* up with the last group of changes from the other file
+* that it can align with.
+*/
while (end_matching_other < end) {
rchg[--start] = 1;
rchg[--end] = 0;
@@ -561,23 +578,6 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long 
flags) {
io--;
}
}
-
-   /*
-* If a group can be moved back and forth, see if there is a
-* blank line in the moving space. If there is a blank line,
-* make sure the last blank line is the end of the group.
-*
-* As we already shifted the group forward as far as possible
-* in the earlier loop, we need to shift it back only if at all.
-*/
-   if ((flags & XDF_COMPACTION_HEURISTIC) && blank_lines) {
-   while (start > 0 &&
-  !is_blank_line(recs, end - 1, flags) &&
-  recs_match(recs, start - 1, end - 1, flags)) {
-   rchg[--start] = 1;
-   rchg[--end] = 0;
-   }
-   }
}
 
return 0;
-- 
2.8.1

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

[PATCH 2/8] xdl_change_compact(): clarify code

[Michael Haggerty]

Write things out a bit longer but less cryptically. Add more comments.

Signed-off-by: Michael Haggerty 
---
I find the loops in the old code, with unfamiliar patterns of embedded
increment/decrement operators, confusing, and I think that writing
things out a little bit more verbosely (and with more comments) makes
it much easier to read the code and be sure that it is correct.
The compiled code and performance shouldn't be affected materially.

 xdiff/xdiffi.c | 106 +++--
 1 file changed, 73 insertions(+), 33 deletions(-)

diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index ff7fc42..a0a485c 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -434,8 +434,14 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long 
flags) {
 * not need index bounding since the array is prepared with
 * a zero at position -1 and N.
 */
-   for (; i < nrec && !rchg[i]; i++)
-   while (rchgo[io++]);
+   for (; i < nrec && !rchg[i]; i++) {
+   /* skip over any changed lines in the other file... */
+   while (rchgo[io])
+   io++;
+
+   /* ...plus one non-changed line. */
+   io++;
+   }
if (i == nrec)
break;
 
@@ -444,45 +450,70 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, 
long flags) {
 * and find the end of it, on both to-be-compacted and other 
file
 * indexes (i and io).
 */
-   start = i;
-   for (i++; rchg[i]; i++);
-   for (; rchgo[io]; io++);
+   start = i++;
+
+   while (rchg[i])
+   i++;
+
+   while (rchgo[io])
+  io++;
 
do {
groupsize = i - start;
+
+   /*
+* Are there any blank lines that could appear as the 
last
+* line of this group?
+*/
blank_lines = 0;
 
/*
-* If the line before the current change group, is 
equal to
-* the last line of the current change group, shift 
backward
-* the group.
+* While the line before the current change group is 
equal
+* to the last line of the current change group, shift 
the
+* group backward.
 */
while (start > 0 && recs_match(recs, start - 1, i - 1, 
flags)) {
rchg[--start] = 1;
rchg[--i] = 0;
 
/*
-* This change might have joined two change 
groups,
-* so we try to take this scenario in account 
by moving
-* the start index accordingly (and so the 
other-file
-* end-of-group index).
+* This change might have joined two change 
groups.
+* If so, move the start index to the beginning 
of
+* the combined group:
 */
-   for (; rchg[start - 1]; start--);
-   while (rchgo[--io]);
+   while (rchg[start - 1])
+   start--;
+
+   /*
+* Move the other file index past a non-changed
+* line...
+*/
+   io--;
+
+   /* ...and also past any changed lines: */
+   while (rchgo[io])
+   io--;
}
 
-   /*
-* Record the end-of-group position in case we are 
matched
-* with a group of changes in the other file (that is, 
the
-* change record before the end-of-group index in the 
other
-* file is set).
-*/
-   ixref = rchgo[io - 1] ? i : nrec;
+   if (rchgo[io - 1]) {
+   /*
+* This change is matched to a group of changes 
in
+* the other file. Record the end-of-group
+* position:
+*/
+   ixref = i;
+   } else {
+ 

[PATCH 6/8] xdl_change_compact(): keep track of the earliest end

[Michael Haggerty]

This makes it easier to detect whether shifting is possible, and will
also make the next change easier.

Signed-off-by: Michael Haggerty 
---
 xdiff/xdiffi.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 66129db..34f021a 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -414,7 +414,8 @@ static int recs_match(xrecord_t **recs, long ixs, long ix, 
long flags)
 }
 
 int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
-   long start, end, io, end_matching_other, groupsize, nrec = xdf->nrec;
+   long start, end, earliest_end, end_matching_other;
+   long io, groupsize, nrec = xdf->nrec;
char *rchg = xdf->rchg, *rchgo = xdfo->rchg;
unsigned int blank_lines;
xrecord_t **recs = xdf->recs;
@@ -516,6 +517,8 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long 
flags) {
end_matching_other = -1;
}
 
+   earliest_end = end;
+
/*
 * Now shift the group forward as long as the first line
 * of the current change group is equal to the line 
after
@@ -547,6 +550,9 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long 
flags) {
}
} while (groupsize != end - start);
 
+   if (end == earliest_end)
+   continue; /* no shifting is possible */
+
if ((flags & XDF_COMPACTION_HEURISTIC) && blank_lines) {
/*
 * Compaction heuristic: if a group can be moved back 
and
-- 
2.8.1

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

[PATCH 5/8] xdl_change_compact(): fix compaction heuristic to adjust io

[Michael Haggerty]

The code branch used for the compaction heuristic incorrectly forgot to
keep io in sync while the group was shifted. I think that could have
led to reading past the end of the rchgo array.

Signed-off-by: Michael Haggerty 
---
I didn't actually try to verify the presence of a bug, because it
seems like more work than worthwhile. But here is my reasoning:

If io is not decremented correctly during one iteration of the outer
`while` loop, then it will loose sync with the `end` counter. In
particular it will be too large.

Suppose that the next iterations of the outer `while` loop (i.e.,
processing the next block of add/delete lines) don't have any sliders.
Then the `io` counter would be incremented by the number of
non-changed lines in xdf, which is the same as the number of
non-changed lines in xdfo that *should have* followed the group that
experienced the malfunction. But since `io` was too large at the end
of that iteration, it will be incremented past the end of the
xdfo->rchg array, and will try to read that memory illegally.

 xdiff/xdiffi.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index c67cfe3..66129db 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -562,6 +562,10 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long 
flags) {
   recs_match(recs, start - 1, end - 1, flags)) {
rchg[--start] = 1;
rchg[--end] = 0;
+
+   io--;
+   while (rchgo[io])
+   io--;
}
} else if (end_matching_other != -1) {
/*
-- 
2.8.1

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

[PATCH 7/8] is_blank_line: take a single xrecord_t as argument

[Michael Haggerty]

There is no reason for it to take an array and index as argument, as it
only accesses a single element of the array.

Signed-off-by: Michael Haggerty 
---
 xdiff/xdiffi.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 34f021a..7518cd5 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -400,9 +400,9 @@ static xdchange_t *xdl_add_change(xdchange_t *xscr, long 
i1, long i2, long chg1,
 }
 
 
-static int is_blank_line(xrecord_t **recs, long ix, long flags)
+static int is_blank_line(xrecord_t *rec, long flags)
 {
-   return xdl_blankline(recs[ix]->ptr, recs[ix]->size, flags);
+   return xdl_blankline(rec->ptr, rec->size, flags);
 }
 
 static int recs_match(xrecord_t **recs, long ixs, long ix, long flags)
@@ -525,7 +525,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long 
flags) {
 * the current change group.
 */
while (end < nrec && recs_match(recs, start, end, 
flags)) {
-   blank_lines += is_blank_line(recs, end, flags);
+   blank_lines += is_blank_line(recs[end], flags);
 
rchg[start++] = 0;
rchg[end++] = 1;
@@ -564,7 +564,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long 
flags) {
 * backward shifts, not forward ones.
 */
while (start > 0 &&
-  !is_blank_line(recs, end - 1, flags) &&
+  !is_blank_line(recs[end - 1], flags) &&
   recs_match(recs, start - 1, end - 1, flags)) {
rchg[--start] = 1;
rchg[--end] = 0;
-- 
2.8.1

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

Re: [PATCH v4 01/12] pkt-line: extract set_packet_header()

[Lars Schneider]

> On 03 Aug 2016, at 22:18, Junio C Hamano  wrote:
> 
> larsxschnei...@gmail.com writes:
> 
>> 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.
> 
> Didn't I say that this is a bad idea already in an earlier review?

Yes, but in that earlier version I made this function *publicly*
available. In this patch the function is only available and used
within pkt-line.c.


> The only reason why you want it, together with direct_packet_write()
> (which I think is another bad idea), is because you use
> packet_buf_write() to create a "" in a buf in the
> usercode in step 11/12 like this:
> 
> + packet_buf_write(, "command=%s\n", filter_type);
> + ret = !direct_packet_write(process->in, nbuf.buf, nbuf.len, 1);
> 
> which would be totally unnecessary if you just did strbuf_addf()
> into nbuf and used packet_write() like everybody else does.

The usercode in step 11/12 could use packet_buf_write(). I am not
worried about performance here. What I am worried about is that
packet_buf_write() dies on error. Since direct_packet_write()
has a "gentle" parameter in can handle these cases. This is important
because a filter might be configured as "required=false" and then
errors are OK.

Would you prefer to see a packet_buf_write_gently() instead?

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

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

[Junio C Hamano]

Jeff Hostetler  writes:

> + else if (!strcmp(arg, "v1") || !strcmp(arg,"1"))

Missing SP between arg, and "1" here (FYI, editing it here to fix
would affect a later patch that has this line in the context).

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

Re: [PATCH v4 01/12] pkt-line: extract set_packet_header()

[Jeff King]

On Wed, Aug 03, 2016 at 05:12:21PM -0400, Jeff King wrote:

> The alternative is to hand-code it, which is what send_sideband() does
> (it uses xsnprintf("%04x") to do the hex formatting, though).

After seeing that, I wondered why we need set_packet_header() at all.
But we do for the case when we are filling in the size at the start of a
buffer, because xsnprintf() will write an extra NUL that we do not care
about. send_sideband() is happy to then overwrite it with data, but
code (like format_packet) that computes the buffer, then fills in the
size, must avoid overwriting the first byte of the buffer.

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

[Lars Schneider]

> On 03 Aug 2016, at 22:29, Junio C Hamano  wrote:
> 
> larsxschnei...@gmail.com writes:
> 
>> +#define FILTER_CAPABILITIES_CLEAN(1u<<0)
>> +#define FILTER_CAPABILITIES_SMUDGE   (1u<<1)
>> +#define FILTER_SUPPORTS_CLEAN(type)  ((type) & FILTER_CAPABILITIES_CLEAN)
>> +#define FILTER_SUPPORTS_SMUDGE(type) ((type) & FILTER_CAPABILITIES_SMUDGE)
> 
> I would expect a lot shorter names as these are file-local;
> CAP_CLEAN and CAP_SMUDGE, perhaps, _WITHOUT_ "supports BLAH" macros?
> 
>   if (FILTER_SUPPORTS_CLEAN(type))
> 
> is not all that more readable than
> 
>   if (CAP_CLEAN & type)

OK. I will change that.


>> +struct cmd2process {
>> +struct hashmap_entry ent; /* must be the first member! */
>> +int supported_capabilities;
>> +const char *cmd;
>> +struct child_process process;
>> +};
>> +
>> +static int cmd_process_map_initialized = 0;
>> +static struct hashmap cmd_process_map;
> 
> Don't initialize statics to 0 or NULL.

OK, statics are initialized implicitly to 0.
I will fix it.


>> +static int cmd2process_cmp(const struct cmd2process *e1,
>> +   const struct cmd2process *e2,
>> +   const void *unused)
>> +{
>> +return strcmp(e1->cmd, e2->cmd);
>> +}
>> +
>> +static struct cmd2process *find_multi_file_filter_entry(struct hashmap 
>> *hashmap, const char *cmd)
>> +{
>> +struct cmd2process key;
>> +hashmap_entry_init(, strhash(cmd));
>> +key.cmd = cmd;
>> +return hashmap_get(hashmap, , NULL);
>> +}
>> +
>> +static void kill_multi_file_filter(struct hashmap *hashmap, struct 
>> cmd2process *entry)
>> +{
>> +if (!entry)
>> +return;
>> +sigchain_push(SIGPIPE, SIG_IGN);
>> +close(entry->process.in);
>> +close(entry->process.out);
>> +sigchain_pop(SIGPIPE);
>> +finish_command(>process);
> 
> I wonder if we want to diagnose failures from close(), which is a
> lot more interesting than usual because these are connected to
> pipes.

In this particular case we kill the filter. That means some error 
already happened, therefore the result wouldn't be of interest
anymore, I think. Wrong?

The other case is the proper shutdown (see 12/12). However, in
that case Git is already exiting and therefore I wonder what
we would do with a "close" error?


>> +static int apply_multi_file_filter(const char *path, const char *src, 
>> size_t len,
>> +   int fd, struct strbuf *dst, const char 
>> *cmd,
>> +   const int wanted_capability)
>> +{
>> +int ret = 1;
>> + ...
>> +if (!(wanted_capability & entry->supported_capabilities))
>> +return 1;  // it is OK if the wanted capability is not supported
> 
> No // comment please.

OK!


>> +filter_result = packet_read_line(process->out, NULL);
>> +ret = !strcmp(filter_result, "result=success");
>> +
>> +done:
>> +if (ret) {
>> +strbuf_swap(dst, );
>> +} else {
>> +if (!filter_result || strcmp(filter_result, "result=reject")) {
>> +// Something went wrong with the protocol filter. Force 
>> shutdown!
>> +error("external filter '%s' failed", cmd);
>> +kill_multi_file_filter(_process_map, entry);
>> +}
>> +}
>> +strbuf_release();
>> +return ret;
>> +}
> 
> I think this was already pointed out in the previous review by Peff,
> but a variable "ret" that says "0 is bad" somehow makes it hard to
> follow the code.  Perhaps rename it to "int error", flip the meaning,
> and if the caller wants this function to return non-zero on success
> flip the polarity in the return statement itself, i.e. "return !errors",
> may make it easier to follow?

This follows the existing filter function. Please see Peff's later
reply here:

"So I'm not sure if changing them is a good idea. I agree with you that
it's probably inviting confusion to have the two sets of filter
functions have opposite return codes. So I think I retract my
suggestion. :)"

http://public-inbox.org/git/20160728133523.GB21311%40sigill.intra.peff.net/

That's why I kept it the way it is. If you prefer the "!errors" approach
then I will change that.


Thanks for looking at the patch,
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 v4 11/12] convert: add filter..process option

[Lars Schneider]

> On 03 Aug 2016, at 19:45, Junio C Hamano  wrote:
> 
> larsxschnei...@gmail.com writes:
> 
>> packet:  git< git-filter-protocol\n
>> packet:  git< version=2\n
>> packet:  git< capabilities=clean smudge\n
> 
> During the discussion on the future of pack-protocol, it was pointed
> out that having to shove all capabilities on a single line/packet
> was one of the things we would want to fix in the current protocol
> when we revamp to v2.  As this exhange between the convert machinery
> and an external process is a brand new one, I do not think you want
> to mimic the limitation in the current pack protocol like this; the
> limitation mostly came from the constraint that we cannot break
> existing pack protocol clients and servers before we extended the
> protocol to add capabilities.
> 
> You may not foresee that the caps won't grow very long beyond
> clean/smudge right now, just like we did not foresee that we would
> wish to be able to convey a lot longer capability values to the
> other side when we added the capability exchange to the pack
> protocol, so "but but but we will never have that many" is not a
> good counter-argument.

OK. Is this the v2 discussion you are referring to?
http://public-inbox.org/git/1461972887-22100-1-git-send-email-sbeller%40google.com/

What format do you suggest?

packet:  git< git-filter-protocol\n
packet:  git< version=2\n
packet:  git< capability=clean\n
packet:  git< capability=smudge\n
packet:  git< 

or

packet:  git< git-filter-protocol\n
packet:  git< version=2\n
packet:  git< capability\n
packet:  git< clean\n
packet:  git< smudge\n
packet:  git< 

or  ... ?

I would prefer the first one, I think.

- 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 v4 3/4] submodule: support running in multiple worktree setup

[Stefan Beller]

On Wed, Jul 27, 2016 at 8:40 AM, Duy Nguyen  wrote:
> On Tue, Jul 26, 2016 at 8:15 PM, Stefan Beller  wrote:
>>> How to store stuff in .git is the implementation details that the user
>>> does not care about.
>>
>> They do unfortunately. :(
>
> Well.. i mean the structure of .git. If .git gets big, yeah many
> people will get pissed.
>
>> My sudden interest in worktrees came up when I learned the
>> `--reference` flag for submodule operations is broken for
>> our use case, and instead of fixing the `--reference` flag,
>> I think the worktree approach is generally saner (i.e. with the
>
> I don't know exactly what that --reference problem is, but keep in
> mind you still have to support single-worktree use case. If it's
> broken in single-worktree, somebody still has to fix it.

So --reference let's you point to a directory such that you can clone
with less data transmission (borrow objects from the local reference).

For submodules this is not per submodule, i.e.

  git clone --recursive --reference 

will only look at that  to borrow for the superproject and all submodules.
But submodules are usually different projects, so you don't find their objects
in the superprojects reference path.

One way out would be to extend the path appropriately (assuming the same
submodule structure in the reference repository).

Another way would be to extend the reference mechanism to look for
objects in the given path and any submodule of that path. Then the submodule
layout can change and --reference is still super effective.

My chose way was to look at the submodule support for worktrees, as that
will hopefully be less brittle w.r.t. gc eventually.

>
>> The current workflow is setup that way because historically you had
>> the submodules .git dir inside the submodule, which would be gone
>> if you deleted a submodule. So if you later checkout an earlier version'
>> that had a submodule, you are missing the objects and more importantly
>> configuration where to get them from.
>>
>> This is now fixed by keeping the actual submodules git dir inside
>> the superprojects git dir.
>
> Hmm.. sounds good, but I'm no judge when it comes to submodules :)

yeah I'll try to get feedback from the submodule people. :)

>
>>> Hmm.. I didn't realize this. But then I have never given much thought
>>> about submodules, probably because I have an alternative solution for
>>> it (or some of its use cases) anyway :)
>>
>> What is that?
>
> Narrow clone (making progress but not there yet). I know it does not
> cover all cases (e.g. submodule can provide separate access control,
> and even using different dvcs system in theory).

heh, ok. Yeah ACLs are the big thing here, so we'd rather go with submodules.

>
>>> OK so it's already a problem. But if we keep sharing submodule stuff
>>> in .git/config, there's a _new_ problem: when you "submodule init" a
>>> worktree, .git/config is now tailored for the current worktree, when
>>> you move back to the previous worktree, you need to "submodule init"
>>> again.
>>
>> "Moving back" sounds like you use the worktree feature for short lived
>> things only. (e.g. in the man page you refer to the hot fix your boss wants
>> you to make urgently).
>>
>> I thought the worktree feature is more useful for long term "branches",
>> e.g. I have one worktree of git now that tracks origin/master so I can
>> use that to "make install" to repair my local broken version of git.
>
> I use it for both. Sometimes you just want to fix something and not
> mess up your current worktree.

I tried worktrees in my daily workflow and the issue for me is my editor
that is worktree agnostic.  As I tried using worktree for different git related
patch series', the set of files I need to look at are the same in the
different work trees

When switching branches the files are still at the same place, such that
the editor, that has a bunch of files open, will just reload the files and you
don't need to open/close files in the editor.
With worktrees you need to open/close all files that you intend to touch in
that worktree, which I dislike as an extra step.

>
>> (I could have a worktree "continuous integration", where I only run tests
>> in. I could have one worktree for Documentation changes only.)
>>
>> This long lived stuff probably doesn't make sense for the a single
>> repository,
>
> It does. You can switch branches in all worktrees. I have a worktree
> specifically for building mingw32 stuff (separate config.mak and
> stuff). When I'm done with a branch on my normal worktree, I could
> move over there, check out the same branch then try mingw32 build. If
> it fails I can fix it right right there and update the branch. When
> everything is ok, I just move back to my normal worktree and continue.

So you use different worktrees for different purposes i.e. editing always
happens in the same, but testing or real hot fixes go into a separate
worktree?


>> So instead of cloning a submodule 

Re: [PATCH v4 03/12] pkt-line: add packet_flush_gentle()

[Jeff King]

On Wed, Aug 03, 2016 at 06:42:16PM +0200, larsxschnei...@gmail.com wrote:

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

Our normal convention would be "0" for success, "-1" for failure.

I see write_or_whine_pipe(), which you use here, has a bizarre "0 for
failure, 1 for success", but that nobody actually checks it.

I actually think you probably don't want to use write_or_whine_pipe()
here. It does two things:

  1. It writes to stderr unconditionally. But if you are doing a
 "gently" form, then you probably don't want unconditional errors.
 Since the point of not dying is that you could presumably recover
 in some way, or do some other more intelligent action.

 The existing callers of write_or_whine_pipe() are all in the trace
 code. Their use is not "let's handle an error", but "we _would_ die
 except that this is low-priority debugging code that should not
 interrupt the normal flow". So there it at least makes sense to
 unconditionally complain to stderr, but not to die().

 For your series, I don't think that is true (and especially for
 most potential callers of a generic "gently flush the packet"
 function).

  2. It calls check_pipe(), which will turn EPIPE into death-by-SIGPIPE
 (in case you had for some reason ignored SIGPIPE).

 But I think that's the opposite of what you want. You know you're
 writing to a pipe, and I would think EPIPE is the most common
 reason that your writes would fail (i.e., the helper unexpectedly
 died while you were writing to it).

 So you would want to explicitly ignore SIGPIPE while talking to the
 helper, and then handle EPIPE just as any other error.

Thinking about (2), I'd go so far as to say that the trace actually
should just be using:

  if (write_in_full(...) < 0)
warning("unable to write trace to ...: %s", strerror(errno));

and we should get rid of write_or_whine_pipe entirely.

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

[Jeff King]

On Wed, Aug 03, 2016 at 01:57:09PM -0700, Junio C Hamano wrote:

> All bugs are from my original, I think.  Here is a proposed squash.
> 
>  * This shouldn't make much difference for @@PAGER_ENV@@, but not
>quoting the default assignment, i.e.
> 
>   : "${VAR=VAL}" && export VAR
> 
>is bad manners.  cf. 01247e02 (sh-setup: enclose setting of
>${VAR=default} in double-quotes, 2016-06-19)
> 
>  * Again, this shouldn't make much difference for PAGER_ENV, but
>let's follow the "reset per iteration, release at the end"
>pattern to avoid repeated allocation.
> 
>  * Let's avoid a hand-rolled "skip blanks" loop and let strspn() do
>it.

Sounds good.

> diff --git a/pager.c b/pager.c
> index cd1ac54..7199c31 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -66,25 +66,22 @@ const char *git_pager(int stdout_is_tty)
>  static void setup_pager_env(struct argv_array *env)
>  {
>   const char *pager_env = PAGER_ENV;
> + struct strbuf buf = STRBUF_INIT;
>  
>   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);

I thought you'd need a strbuf_reset() here, but I guess it is handled by
the one at the end of the loop. That's OK because there are no other
loop exits, though IMHO putting it at the top makes the reusable-strbuf
idiom easier to understand.

-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 v4 2/8] status: cleanup API to wt_status_print

[Junio C Hamano]

Jeff Hostetler  writes:

> From: Jeff Hostetler 
>
> Refactor the API between builtin/commit.c and wt-status.[ch].
> Hide details of the various wt_*status_print() routines inside
> wt-status.c behind a single (new) wt_status_print() routine
> and eliminate the switch statements from builtin/commit.c
>
> This will allow us to more easily add new status formats
> and isolate that within wt-status.c
>
> Signed-off-by: Jeff Hostetler 
> Signed-off-by: Jeff Hostetler 

Again, are these the same person?  I won't repeat this for the
remainder of the series.

> diff --git a/wt-status.h b/wt-status.h
> index 2023a3c..a859a12 100644
> --- a/wt-status.h
> +++ b/wt-status.h
> @@ -43,6 +43,15 @@ struct wt_status_change_data {
>   unsigned new_submodule_commits : 1;
>  };
>  
> + enum wt_status_format {
> + STATUS_FORMAT_NONE = 0,
> + STATUS_FORMAT_LONG,
> + STATUS_FORMAT_SHORT,
> + STATUS_FORMAT_PORCELAIN,
> +
> + STATUS_FORMAT_UNSPECIFIED
> + };

Is it your editor, or is it your MUA?  This definition is indented
by one SP, which is funny.

Also throughout the series, I saw a handful of blank lines that
should be empty but are not (e.g. three tabs and nothing else on a
new line).  I've fixed them up all but I won't be sending an
interdiff just for them, so please make sure they won't resurface
when/if you reroll.

>  struct wt_status {
>   int is_initial;
>   char *branch;
> @@ -66,6 +75,8 @@ struct wt_status {
>   int show_branch;
>   int hints;
>  
> + enum wt_status_format status_format;
> +
>   /* These are computed during processing of the individual sections */
>   int commitable;
>   int workdir_dirty;
> @@ -99,6 +110,7 @@ struct wt_status_state {
>  void wt_status_truncate_message_at_cut_line(struct strbuf *);
>  void wt_status_add_cut_line(FILE *fp);
>  void wt_status_prepare(struct wt_status *s);
> +void wt_status_print(struct wt_status *s);
>  void wt_status_collect(struct wt_status *s);
>  void wt_status_get_state(struct wt_status_state *state, int 
> get_detached_from);
>  int wt_status_check_rebase(const struct worktree *wt,
> @@ -106,10 +118,6 @@ int wt_status_check_rebase(const struct worktree *wt,
>  int wt_status_check_bisect(const struct worktree *wt,
>  struct wt_status_state *state);
>  
> -void wt_longstatus_print(struct wt_status *s);
> -void wt_shortstatus_print(struct wt_status *s);
> -void wt_porcelain_print(struct wt_status *s);
> -

I think I agreed during the previous review round that shifting the
division of labor boundary between the command and wt-status.[ch]
this way, to have the latter do more printing, is a good idea, and I
still do.

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

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

[Junio C Hamano]

Jeff Hostetler  writes:

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

We tend to write "Rename the various...", as if you the patch author
is giving an order to "become so" to the codebase, or "make it so"
to the maintainer.

Other than that, this step looks straight-forward, and I agree with
the hope that this will reduce confusion that readers may have while
reading a future version of this code.

> This will hopefully reduce confusion as other status
> formats are added in the future.
>
> Signed-off-by: Jeff Hostetler 
> Signed-off-by: Jeff Hostetler 

Hmm, are these physically the same people?  If so, which one do you
want to be known as?

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 v4 07/12] run-command: add clean_on_exit_handler

[Jeff King]

On Wed, Aug 03, 2016 at 06:42:20PM +0200, larsxschnei...@gmail.com wrote:

> From: Lars Schneider 
> 
> Some commands might need to perform cleanup tasks on exit. Let's give
> them an interface for doing this.
> 
> Please note, that the cleanup callback is not executed if Git dies of a
> signal. The reason is that only "async-signal-safe" functions would be
> allowed to be call in that case. Since we cannot control what functions
> the callback will use, we will not support the case. See 507d7804 for
> more details.

I'm not clear on why we want this cleanup filter. It looks like you use
it in the final patch to send an explicit shutdown to any filters we
start. But I see two issues with that:

  1. This shutdown may come at any time, and you have no idea what state
 the protocol conversation with the filter is in. You could be in
 the middle of sending another pkt-line, or in a sequence of non-command
 pkt-lines where "shutdown" is not recognized.

  2. If your protocol does bad things when it is cut off in the middle
 without an explicit shutdown, then it's a bad protocol. As you
 note, this patch doesn't cover signal death, nor could it ever
 cover something like "kill -9", or a bug which prevented git from
 saying "shutdown".

 You're much better off to design the protocol so that a premature
 EOF is detected as an error.  For example, if we're feeding file
 data to the filter, and we're worried it might be writing it to
 a data store (like LFS), we would not want it to see EOF and say
 "well, I guess I got all the data; time to store this!". Instead,
 it should know how many bytes are coming, or should have some kind
 of framing so that the sender says "and now you have seen all the
 bytes" (like a pkt-line flush).

 AFAIK, your protocol _does_ do those things sensibly, so this
 explicit shutdown isn't really accomplishing anything.

-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 v4 01/12] pkt-line: extract set_packet_header()

[Jeff King]

On Wed, Aug 03, 2016 at 01:18:55PM -0700, Junio C Hamano wrote:

> larsxschnei...@gmail.com writes:
> 
> > 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.
> 
> Didn't I say that this is a bad idea already in an earlier review?
> 
> The only reason why you want it, together with direct_packet_write()
> (which I think is another bad idea), is because you use
> packet_buf_write() to create a "" in a buf in the
> usercode in step 11/12 like this:
> 
> + packet_buf_write(, "command=%s\n", filter_type);
> + ret = !direct_packet_write(process->in, nbuf.buf, nbuf.len, 1);
> 
> which would be totally unnecessary if you just did strbuf_addf()
> into nbuf and used packet_write() like everybody else does.
> 
> Puzzled.  Why are steps 01/12 and 02/12 an improvement?

I think it is an attempt to avoid the extra memcpy() of the bytes into
another packet buffer.

I notice that the solution does still end up a using a double-write() in
some cases, though.  I was curious if this made any difference, though,
so I wrote a short test program:

-- >8 --
#include 
#include 

int main(int argc, char **argv)
{
int type;

if (argv[1] && !strcmp(argv[1], "prepend"))
type = 0; /* size prepended to buffer */
else if (argv[1] && !strcmp(argv[1], "write"))
type = 1;
else if (argv[1] && !strcmp(argv[1], "memcpy"))
type = 2;
else
return 1;

while (1) {
char buf[65520];
int r = read(0, buf + 4, sizeof(buf));
if (r <= 0)
break;
if (!type) {
memcpy(buf, "1234", 4);
write(1, buf, r + 4);
} else if (type == 1) {
write(1, "1234", 4);
write(1, buf + 4, r);
} else if (type == 2) {
char packet[sizeof(buf) + 4];
memcpy(packet, "1234", 4);
memcpy(packet + 4, buf + 4, r);
write(1, packet, r + 4);
}
}
return 0;
}
-- >8 --

We'd expect "prepend" to be the fastest, as it does a single write and
zero-copy. And then it is a question of whether the double-write is
worse than the extra memcpy.

On Linux, feeding 100MB of zeroes into stdin, I got (best-of-five):

  - prepend: 11ms
  - write: 11ms
  - memcpy: 15ms

So it _does_ make a difference to avoid the memcpy, though 4ms per 100MB
does not seem like it is probably worth caring about. The double-write
also gets worse if you use a smaller buffer size (e.g., if you drop to
4K, that adds back in about 4ms of overhead because you're calling
write() a lot more times).

The cost of write() may vary on other platforms, but the cost of memcpy
generally shouldn't. So I'm inclined to say that it is not really worth
micro-optimizing the interface.

I think the other issue is that format_packet() only lets you send
string data via "%s", so it cannot be used for arbitrary data that may
contain NULs. So we do need _some_ other interface to let you send a raw
data packet, and it's going to look similar to the direct_packet_write()
thing.

The alternative is to hand-code it, which is what send_sideband() does
(it uses xsnprintf("%04x") to do the hex formatting, though).

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

[Junio C Hamano]

Eric Wong  writes:

> Junio C Hamano  wrote:
>> All bugs are from my original, I think.  Here is a proposed squash.
>
> Thanks, I'll take the git-sh-setup changes.
>
> I actually just rewrote setup_pager_env using split_cmdline and
> eliminated all the scary (to me) pointer arithmetic and avoided
> strbuf, too.

I actually do not have much faith in split_cmdline() in that I
cannot quite read and follow the flow of the logic in its
implementation, but it already is used widely so it must be OK, so I
am fine if you use it as a black-box to make this code simpler ;-)

I'll drop the squash then and replace it with your version when it
comes.

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

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

[Eric Wong]

Junio C Hamano  wrote:
> All bugs are from my original, I think.  Here is a proposed squash.

Thanks, I'll take the git-sh-setup changes.

I actually just rewrote setup_pager_env using split_cmdline and
eliminated all the scary (to me) pointer arithmetic and avoided
strbuf, too.

Unfortunately, my Internet connection on that machine dropped
before I could finish testing and pushing :<

This was also the machine running http://czquwvybam4bgbro.onion/git/ :<
--
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]

Jeff King  writes:

> On Mon, Aug 01, 2016 at 09:49:37PM +, Eric Wong wrote:
>
> You'd want:
>
>   argv_array_pushf(env, "%.*s", (int)(cp - pager_env), pager_env);
>
> Also:
>
>> +strbuf_reset();
>
> should this be strbuf_release()? If we didn't follow the conditional
> above (because getenv() told us the variable was already set), then we
> would not do do the detach/release there, and would finish the loop with
> memory still allocated by "buf".

All bugs are from my original, I think.  Here is a proposed squash.

 * This shouldn't make much difference for @@PAGER_ENV@@, but not
   quoting the default assignment, i.e.

: "${VAR=VAL}" && export VAR

   is bad manners.  cf. 01247e02 (sh-setup: enclose setting of
   ${VAR=default} in double-quotes, 2016-06-19)

 * Again, this shouldn't make much difference for PAGER_ENV, but
   let's follow the "reset per iteration, release at the end"
   pattern to avoid repeated allocation.

 * Let's avoid a hand-rolled "skip blanks" loop and let strspn() do
   it.


 git-sh-setup.sh |  2 +-
 pager.c | 15 ++-
 2 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index b6b75e9..cda32d0 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -163,7 +163,7 @@ git_pager() {
for vardef in @@PAGER_ENV@@
do
var=${vardef%%=*}
-   eval ": \${$vardef} && export $var"
+   eval ": \"\${$vardef}\" && export $var"
done
 
eval "$GIT_PAGER" '"$@"'
diff --git a/pager.c b/pager.c
index cd1ac54..7199c31 100644
--- a/pager.c
+++ b/pager.c
@@ -66,25 +66,22 @@ const char *git_pager(int stdout_is_tty)
 static void setup_pager_env(struct argv_array *env)
 {
const char *pager_env = PAGER_ENV;
+   struct strbuf buf = STRBUF_INIT;
 
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));
-   }
+   if (!getenv(buf.buf))
+   argv_array_pushf(env, "%.*s",
+(int)(cp - pager_env), pager_env);
+   pager_env = cp + strspn(cp, " ");
strbuf_reset();
-   while (*cp && *cp == ' ')
-   cp++;
-   pager_env = cp;
}
+   strbuf_release();
 }
 
 void prepare_pager_args(struct child_process *pager_process, const char *pager)
--
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/PATCH v11 09/13] bisect--helper: `bisect_write` shell function in C

[Pranit Bauva]

Hey Junio,

On Wed, Aug 3, 2016 at 3:47 AM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> Pranit Bauva  writes:
>>
>>> Reimplement the `bisect_write` shell function in C and add a
>>> `bisect-write` subcommand to `git bisect--helper` to call it from
>>> git-bisect.sh
>>
>> Up to around this step we've seen these patches well enough and I
>> think with another reroll or two, they are in good enough shape to
>> be split out and frozen for 'next'.  We may not be there quite yet,
>> but I think we are getting pretty close.
>>
>> Thanks.
>
> By the way, this series applied as a whole seems to break t6030.

This is only because of argument handling problem. I seemed to have
mentioned it in patch 13/13 and also gave the output of the tests.

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

Re: [RFC/PATCH v11 09/13] bisect--helper: `bisect_write` shell function in C

[Pranit Bauva]

Hey Junio,

On Wed, Aug 3, 2016 at 1:55 AM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>> Reimplement the `bisect_write` shell function in C and add a
>> `bisect-write` subcommand to `git bisect--helper` to call it from
>> git-bisect.sh
>
> Up to around this step we've seen these patches well enough and I
> think with another reroll or two, they are in good enough shape to
> be split out and frozen for 'next'.  We may not be there quite yet,
> but I think we are getting pretty close.

Thanks for taking out your time to review this series. I will follow
up with the patches in a day or so. I think the first 11, many things
wouldn't change (except for your reviews). Most of the changes would
happen in 12 and 13 and more specifically the argument handling.

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

Re: [RFC/PATCH v11 13/13] bisect--helper: `bisect_start` shell function partially in C

[Pranit Bauva]

Hey Junio,

On Wed, Aug 3, 2016 at 1:49 AM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>> +static int bisect_start(struct bisect_terms *terms, int no_checkout,
>> + const char **argv, int argc)
>> +{
>> + int i, j, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
>> + int flag;
>> + struct string_list revs = STRING_LIST_INIT_DUP;
>> + struct string_list states = STRING_LIST_INIT_DUP;
>> + struct strbuf start_head = STRBUF_INIT;
>> + const char *head;
>> + unsigned char sha1[20];
>> + FILE *fp;
>> + struct object_id oid;
>> +
>> + if (is_bare_repository())
>> + no_checkout = 1;
>> +
>> + for(i = 0; i < argc; i++) {
>
> SP after for.

Sure!

>> + if (!strcmp(argv[i], "--")) {
>> + has_double_dash = 1;
>> + break;
>> + }
>> + if (!strcmp(argv[i], "--term-good")) {
>> + must_write_terms = 1;
>> + strbuf_reset(>term_good);
>> + strbuf_addstr(>term_good, argv[++i]);
>> + break;
>> + }
>> + if (!strcmp(argv[i], "--term-bad")) {
>> + must_write_terms = 1;
>> + strbuf_reset(>term_bad);
>> + strbuf_addstr(>term_bad, argv[++i]);
>> + break;
>> + }
>
> The original was not careful, either, but what if the user ends the
> command line with "--term-good", without anything after it?

> Also the original is prepared to handle --term-good=boa; because
> this function can be be called directly from the UI (i.e. "git
> bisect start --term-good=boa"), not supporting that form would be
> seen as a regression.


I wanted to discuss this precisely by this RFC. I was initially
thinking of using OPT_ARGUMENT() for bisect_terms() which would in
turn cover up for bisect_start() too. Currently the code does not
support --term-good=boa because it treats --term-good as a boolean Do
you have any other thing in mind?

>> + if (starts_with(argv[i], "--") &&
>> + !one_of(argv[i], "--term-good", "--term-bad", NULL)) {
>> + string_list_clear(, 0);
>> + string_list_clear(, 0);
>> + die(_("unrecognised option: '%s'"), argv[i]);
>> + }
>> + if (get_oid(argv[i], ) || has_double_dash) {
>
> Calling get_oid() alone is insufficient to make sure argv[i] refers
> to an existing object that is a committish.  The "^{commit}" suffix
> in the original is there for a reason.

Yes sure!

>> + string_list_clear(, 0);
>> + string_list_clear(, 0);
>
> You seem to want the revs list really really clean ;-)

Haha! ;) My bad. Will remove the extra line!

>> + die(_("'%s' does not appear to be a valid revision"), 
>> argv[i]);
>> + }
>> + else
>> + string_list_append(, oid_to_hex());
>> + }
>> +
>> + for (j = 0; j < revs.nr; j++) {
>
> Why "j", not "i", as clearly the previous loop has finished at this
> point?  The only reason why replacing "j" with "i" would make this
> function buggy would be if a later part of this function depended on
> the value of "i" when the control left the above loop, but if that
> were the case (I didn't check carefully), such a precious value that
> has long term effect throughout the remainder of the function must
> not be kept in an otherwise throw-away loop counter variable "i".
>
> Introduce a new "int pathspec_pos" and set it to "i" immediately
> after the "for (i = 0; i < argc; i++) { ... }" loop above, perhaps.

I am using i afterwards for writing the arguments to BISECT_NAMES. But
I think it would be better to use pathspec_pos and discard j
altogether. Thanks!
>
>> + struct strbuf state = STRBUF_INIT;
>> + /*
>> +  * The user ran "git bisect start  ", hence
>> +  * did not explicitly specify the terms, but we are already
>> +  * starting to set references named with the default terms,
>> +  * and won't be able to change afterwards.
>> +  */
>> + must_write_terms = 1;
>> +
>> + if (bad_seen)
>> + strbuf_addstr(, terms->term_good.buf);
>> + else {
>> + bad_seen = 1;
>> + strbuf_addstr(, terms->term_bad.buf);
>> + }
>> + string_list_append(, state.buf);
>> + strbuf_release();
>> + }
>
> How about this instead?
>
> /*
>  * that comment block goes here
>  */
> must_write_terms = !!revs.nr;
> for (i = 0; i < revs.nr; i++) {
> if (bad_seen)
> string_list_append(, terms->term_good.buf);
> else
>

Re: [RFC/PATCH v11 11/13] bisect--helper: `bisect_next_check` shell function in C

[Pranit Bauva]

Hey Junio,

On Wed, Aug 3, 2016 at 12:47 AM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>> +static int mark_good(const char *refname, const struct object_id *oid,
>> +  int flag, void *cb_data)
>> +{
>> + int *m_good = (int *)cb_data;
>> + *m_good = 0;
>> + return 0;
>> +}
>
> See below.
>
>> +static int bisect_next_check(const struct bisect_terms *terms,
>> +  const char *current_term)
>> +{
>> + int missing_good = 1, missing_bad = 1;
>
> It is somewhat unusual to start with "assume we are OK" and then
> "it turns out that we are not".
>
>> + char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad.buf);
>> + char *good_glob = xstrfmt("%s*", terms->term_good.buf);
>
> The original runs
>
> git for-each-ref "refs/bisect/$TERM_GOOD-*
>
> but this one lacks the final dash.

My bad. Will include it.

>> + if (ref_exists(bad_ref))
>> + missing_bad = 0;
>> + free(bad_ref);
>> +
>> + for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/",
>> +  (void *) _good);
>> + free(good_glob);
>
> The for-each helper does not return until it iterates over all the
> matching refs, but you are only interested in seeing if at least one
> exists.  It may make sense to return 1 from mark_good() to terminate
> the traversal early.

Seems a better way. Thanks!

>> + if (!missing_good && !missing_bad)
>> + return 0;
>> +
>> + if (!current_term)
>> + return -1;
>> +
>> + if (missing_good && !missing_bad && current_term &&
>> + !strcmp(current_term, terms->term_good.buf)) {
>> + char *yesno;
>> + /*
>> +  * have bad (or new) but not good (or old). We could bisect
>> +  * although this is less optimum.
>> +  */
>> + fprintf(stderr, "Warning: bisecting only with a %s commit\n",
>> + terms->term_bad.buf);
>
> In the original, this message goes through gettext.

Will do.

>> + if (!isatty(0))
>> + return 0;
>> + /*
>> +  * TRANSLATORS: Make sure to include [Y] and [n] in your
>> +  * translation. The program will only accept English input
>> +  * at this point.
>> +  */
>> + yesno = git_prompt(_("Are you sure [Y/n]? "), PROMPT_ECHO);
>> + if (starts_with(yesno, "N") || starts_with(yesno, "n"))
>> + return -1;
>> + return 0;
>> + }
>
> When the control falls into the above if(){} block, the function
> will always return.  It will clarify that this is the end of such a
> logical block to have a blank line here.

Will do.

>> + if (!is_empty_or_missing_file(git_path_bisect_start()))
>> + return error(_("You need to give me at least one good|old and "
>> + "bad|new revision. You can use \"git bisect "
>> + "bad|new\" and \"git bisect good|old\" for "
>> + "that. \n"));
>> + else
>> + return error(_("You need to start by \"git bisect start\". "
>> + "You then need to give me at least one good|"
>> + "old and bad|new revision. You can use \"git "
>> + "bisect bad|new\" and \"git bisect good|old\" "
>> + " for that.\n"));
>
> The i18n on these two messages seem to be different from the
> original, which asks bisect_voc to learn what 'bad' and 'good' are
> called and attempts to use these words from the vocabulary.

I have little idea about i18n. Will look more into it.

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

[PATCHv4 6/7] submodule update: allow '.' for branch value

[Stefan Beller]

Gerrit has a "superproject subscription" feature[1], that triggers a
commit in a superproject that is subscribed to its submodules.
Conceptually this Gerrit feature can be done on the client side with
Git via (except for raciness, error handling etc):

  while [ true ]; do
git -C  submodule update --remote --force
git -C  commit -a -m "Update submodules"
git -C  push
  done

for each branch in the superproject. To ease the configuration in Gerrit
a special value of "." has been introduced for the submodule..branch
to mean the same branch as the superproject[2], such that you can create a
new branch on both superproject and the submodule and this feature
continues to work on that new branch.

Now we find projects in the wild with such a .gitmodules file.
The .gitmodules used in these Gerrit projects do not conform
to Gits understanding of how .gitmodules should look like.
This teaches Git to deal gracefully with this syntax as well.

The redefinition of "." does no harm to existing projects unaware of
this change, as "." is an invalid branch name in Git, so we do not
expect such projects to exist.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 builtin/submodule--helper.c | 18 ++
 t/t7406-submodule-update.sh | 35 ++-
 2 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 9be2c75..f1acc4d 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -912,6 +912,24 @@ static const char *remote_submodule_branch(const char 
*path)
if (!sub->branch)
return "master";
 
+   if (!strcmp(sub->branch, ".")) {
+   unsigned char sha1[20];
+   const char *refname = resolve_ref_unsafe("HEAD", 0, sha1, NULL);
+
+   if (!refname)
+   die(_("No such ref: %s"), "HEAD");
+
+   /* detached HEAD */
+   if (!strcmp(refname, "HEAD"))
+   die(_("Submodule (%s) branch configured to inherit "
+ "branch from superproject, but the superproject "
+ "is not on any branch"), sub->name);
+
+   if (!skip_prefix(refname, "refs/heads/", ))
+   die(_("Expecting a full ref name, got %s"), refname);
+   return refname;
+   }
+
return sub->branch;
 }
 
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 1bb1f43..d7983cf 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -209,9 +209,42 @@ test_expect_success 'submodule update --remote should 
fetch upstream changes' '
)
 '
 
+test_expect_success 'submodule update --remote should fetch upstream changes 
with .' '
+   (
+   cd super &&
+   git config -f .gitmodules submodule."submodule".branch "." &&
+   git add .gitmodules &&
+   git commit -m "submodules: update from the respective 
superproject branch"
+   ) &&
+   (
+   cd submodule &&
+   echo line4a >> file &&
+   git add file &&
+   test_tick &&
+   git commit -m "upstream line4a" &&
+   git checkout -b test-branch &&
+   test_commit on-test-branch
+   ) &&
+   (
+   cd super &&
+   git submodule update --remote --force submodule &&
+   git -C submodule log -1 --oneline >actual
+   git -C ../submodule log -1 --oneline master >expect
+   test_cmp expect actual &&
+   git checkout -b test-branch &&
+   git submodule update --remote --force submodule &&
+   git -C submodule log -1 --oneline >actual
+   git -C ../submodule log -1 --oneline test-branch >expect
+   test_cmp expect actual &&
+   git checkout master &&
+   git branch -d test-branch &&
+   git reset --hard HEAD^
+   )
+'
+
 test_expect_success 'local config should override .gitmodules branch' '
(cd submodule &&
-git checkout -b test-branch &&
+git checkout test-branch &&
 echo line5 >> file &&
 git add file &&
 test_tick &&
-- 
2.9.2.524.gdbd1860

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

[PATCHv4 {6,7}/7] submodule update: allow '.' for branch value

[Stefan Beller]

This replaces the last two commits of  sb/submodule-update-dot-branch.

Thanks Jeff for pointing out the possible segfault.
In the last commit I removed commented code in the test.

Thanks,
Stefan

diff to sb/submodule-update-dot-branch:
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ae88eff..f1acc4d 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -906,6 +906,9 @@ static const char *remote_submodule_branch(const char *path)
git_config(submodule_config, NULL);
 
sub = submodule_from_path(null_sha1, path);
+   if (!sub)
+   return NULL;
+
if (!sub->branch)
return "master";
 
@@ -933,11 +936,16 @@ static const char *remote_submodule_branch(const char 
*path)
 static int resolve_remote_submodule_branch(int argc, const char **argv,
const char *prefix)
 {
+   const char *ret;
struct strbuf sb = STRBUF_INIT;
if (argc != 2)
die("submodule--helper remote-branch takes exactly one 
arguments, got %d", argc);
 
-   printf("%s", remote_submodule_branch(argv[1]));
+   ret = remote_submodule_branch(argv[1]);
+   if (!ret)
+   die("submodule %s doesn't exist", argv[1]);
+
+   printf("%s", ret);
strbuf_release();
return 0;
 }
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 1c4c1f2..d7983cf 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -238,7 +238,6 @@ test_expect_success 'submodule update --remote should fetch 
upstream changes wit
test_cmp expect actual &&
git checkout master &&
git branch -d test-branch &&
-   #~ git -C ../submodule branch -d test-branch &&
git reset --hard HEAD^
)
 '

Stefan Beller (2):
  submodule--helper: add remote-branch helper
  submodule update: allow '.' for branch value

 builtin/submodule--helper.c | 54 -
 git-submodule.sh|  2 +-
 t/t7406-submodule-update.sh | 35 -
 3 files changed, 88 insertions(+), 3 deletions(-)

-- 
2.9.2.524.gdbd1860

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

[PATCHv4 6/7] submodule--helper: add remote-branch helper

[Stefan Beller]

In a later patch we want to enhance the logic for the branch selection.
Rewrite the current logic to be in C, so we can directly use C when
we enhance the logic.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 builtin/submodule--helper.c | 36 +++-
 git-submodule.sh|  2 +-
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index fb90c64..9be2c75 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -899,6 +899,39 @@ static int resolve_relative_path(int argc, const char 
**argv, const char *prefix
return 0;
 }
 
+static const char *remote_submodule_branch(const char *path)
+{
+   const struct submodule *sub;
+   gitmodules_config();
+   git_config(submodule_config, NULL);
+
+   sub = submodule_from_path(null_sha1, path);
+   if (!sub)
+   return NULL;
+
+   if (!sub->branch)
+   return "master";
+
+   return sub->branch;
+}
+
+static int resolve_remote_submodule_branch(int argc, const char **argv,
+   const char *prefix)
+{
+   const char *ret;
+   struct strbuf sb = STRBUF_INIT;
+   if (argc != 2)
+   die("submodule--helper remote-branch takes exactly one 
arguments, got %d", argc);
+
+   ret = remote_submodule_branch(argv[1]);
+   if (!ret)
+   die("submodule %s doesn't exist", argv[1]);
+
+   printf("%s", ret);
+   strbuf_release();
+   return 0;
+}
+
 struct cmd_struct {
const char *cmd;
int (*fn)(int, const char **, const char *);
@@ -912,7 +945,8 @@ static struct cmd_struct commands[] = {
{"relative-path", resolve_relative_path},
{"resolve-relative-url", resolve_relative_url},
{"resolve-relative-url-test", resolve_relative_url_test},
-   {"init", module_init}
+   {"init", module_init},
+   {"remote-branch", resolve_remote_submodule_branch}
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/git-submodule.sh b/git-submodule.sh
index 41aff65..8c5e898 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -614,7 +614,7 @@ cmd_update()
 
if test -n "$remote"
then
-   branch=$(get_submodule_config "$name" branch master)
+   branch=$(git submodule--helper remote-branch "$sm_path")
if test -z "$nofetch"
then
# Fetch remote before determining tracking $sha1
-- 
2.9.2.524.gdbd1860

--
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/PATCH v11 12/13] bisect--helper: `get_terms` & `bisect_terms` shell function in C

[Pranit Bauva]

Hey Junio,

On Wed, Aug 3, 2016 at 12:52 AM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>> +static int bisect_terms(struct bisect_terms *terms, int term_defined)
>> +{
>> + if (get_terms(terms)) {
>> + fprintf(stderr, "no terms defined\n");
>> + return -1;
>> + }
>> + if (!term_defined) {
>> + printf("Your current terms are %s for the old state\nand "
>> +"%s for the new state.\n", terms->term_good.buf,
>> +terms->term_bad.buf);
>> + return 0;
>> + }
>
> In the original, all of the above messages go through gettext; this
> rewrite should do the same.

Sure!

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

Re: [RFC/PATCH v11 10/13] bisect--helper: `check_and_set_terms` shell function in C

[Pranit Bauva]

Hey Junio,

On Wed, Aug 3, 2016 at 12:23 AM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>> Reimplement the `check_and_set_terms` shell function in C and add
>> `check-and-set-terms` subcommand to `git bisect--helper` to call it from
>> git-bisect.sh
>>
>> Using `--check-and-set-terms` subcommand is a temporary measure to port
>> shell function in C so as to use the existing test suite. As more
>> functions are ported, this subcommand will be retired and will be called
>> by some other methods.
>
> I think "this subcommand will be retired but its implementation will
> be called by ..." would clarify the direction.

Sure. That seems better.

>> + if (!no_term_file &&
>> + strcmp(cmd, terms->term_bad.buf) &&
>> + strcmp(cmd, terms->term_good.buf))
>> + return error(_("Invalid command: you're currently in a "
>> + "'%s' '%s' bisect"), terms->term_bad.buf,
>
> This changes a message text, switching from "... good/bad bisect."
> to "... 'good' 'bad' bisect".  Intended?

Nope its not intended but its a mistake from my side. Will rectify.

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

Re: [PATCH v4 11/12] convert: add filter..process option

[Junio C Hamano]

larsxschnei...@gmail.com writes:

> +#define FILTER_CAPABILITIES_CLEAN(1u<<0)
> +#define FILTER_CAPABILITIES_SMUDGE   (1u<<1)
> +#define FILTER_SUPPORTS_CLEAN(type)  ((type) & FILTER_CAPABILITIES_CLEAN)
> +#define FILTER_SUPPORTS_SMUDGE(type) ((type) & FILTER_CAPABILITIES_SMUDGE)

I would expect a lot shorter names as these are file-local;
CAP_CLEAN and CAP_SMUDGE, perhaps, _WITHOUT_ "supports BLAH" macros?

if (FILTER_SUPPORTS_CLEAN(type))

is not all that more readable than

if (CAP_CLEAN & type)



> +struct cmd2process {
> + struct hashmap_entry ent; /* must be the first member! */
> + int supported_capabilities;
> + const char *cmd;
> + struct child_process process;
> +};
> +
> +static int cmd_process_map_initialized = 0;
> +static struct hashmap cmd_process_map;

Don't initialize statics to 0 or NULL.

> +static int cmd2process_cmp(const struct cmd2process *e1,
> +   const struct cmd2process *e2,
> +   const void *unused)
> +{
> + return strcmp(e1->cmd, e2->cmd);
> +}
> +
> +static struct cmd2process *find_multi_file_filter_entry(struct hashmap 
> *hashmap, const char *cmd)
> +{
> + struct cmd2process key;
> + hashmap_entry_init(, strhash(cmd));
> + key.cmd = cmd;
> + return hashmap_get(hashmap, , NULL);
> +}
> +
> +static void kill_multi_file_filter(struct hashmap *hashmap, struct 
> cmd2process *entry)
> +{
> + if (!entry)
> + return;
> + sigchain_push(SIGPIPE, SIG_IGN);
> + close(entry->process.in);
> + close(entry->process.out);
> + sigchain_pop(SIGPIPE);
> + finish_command(>process);

I wonder if we want to diagnose failures from close(), which is a
lot more interesting than usual because these are connected to
pipes.

> +static int apply_multi_file_filter(const char *path, const char *src, size_t 
> len,
> +   int fd, struct strbuf *dst, const char 
> *cmd,
> +   const int wanted_capability)
> +{
> + int ret = 1;
> + ...
> + if (!(wanted_capability & entry->supported_capabilities))
> + return 1;  // it is OK if the wanted capability is not supported

No // comment please.

> + filter_result = packet_read_line(process->out, NULL);
> + ret = !strcmp(filter_result, "result=success");
> +
> +done:
> + if (ret) {
> + strbuf_swap(dst, );
> + } else {
> + if (!filter_result || strcmp(filter_result, "result=reject")) {
> + // Something went wrong with the protocol filter. Force 
> shutdown!
> + error("external filter '%s' failed", cmd);
> + kill_multi_file_filter(_process_map, entry);
> + }
> + }
> + strbuf_release();
> + return ret;
> +}

I think this was already pointed out in the previous review by Peff,
but a variable "ret" that says "0 is bad" somehow makes it hard to
follow the code.  Perhaps rename it to "int error", flip the meaning,
and if the caller wants this function to return non-zero on success
flip the polarity in the return statement itself, i.e. "return !errors",
may make it easier to follow?
--
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 6/7] submodule--helper: add remote-branch helper

[Jeff King]

On Wed, Aug 03, 2016 at 01:11:51PM -0700, Stefan Beller wrote:

> > Coverity complains about "sub" being NULL here, and indeed, it seems
> > like an easy segfault:
> >
> >   $ ./git submodule--helper remote-branch foo
> >   Segmentation fault
> >
> > I guess this should return NULL in that case. But then this...
> 
> I thought about checking for (!sub && !sub->branch) to return "master";
> However I disregarded that implementation as the submodule-config is
> reliable for any correct input. Though we need to handle wrong input as
> well. So how about:
> 
>   if (!sub)
> die(_("A submodule doesn't exist at path %s"), path);
>   if (!sub->branch)
> return "master";

OK by me, as long as the caller of submodule--helper handles the death
reasonably. Since it's an internal helper program, we really only have
to worry about what git-submodule.sh does with it, which is good.

> > would need to handle the NULL return, as well. So maybe "master" or the
> > empty string would be better. I haven't followed the topic closely
> > enough to have an opinion.
> 
> Oh I see, this can turn into a discussion what the API for
> remote_submodule_branch is. I think handling the NULL here and dying makes
> more sense as then we can keep remote_submodule_branch pure to its
> intended use case. (If in the far future we have all the submodule stuff in C,
> we may want to call remote_submodule_branch when we already know
> that the path is valid, so no need to check it in there. Also not dying in 
> there
> is more libified.)

Yep, that makes sense to me, too.

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

[Jakub Narębski]

[This response might have been invalidated by v4]

W dniu 01.08.2016 o 14:18, Lars Schneider pisze:
>> On 30 Jul 2016, at 14:29, Jakub Narębski  wrote:
>> W dniu 30.07.2016 o 01:37, larsxschnei...@gmail.com pisze:

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

Yes, that's much better.

P.S. Yes, this is one of those changes where commit message is much longer
 than the change itself...

-- 
Jakub Narębski

--
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/PATCH v11 02/13] bisect: rewrite `check_term_format` shell function in C

[Pranit Bauva]

Hey Junio,

On Tue, Aug 2, 2016 at 11:01 PM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>> +/*
>> + * Check whether the string `term` belongs to the set of strings
>> + * included in the variable arguments.
>> + */
>> +static int one_of(const char *term, ...)
>> +{
>> + int res = 0;
>> + va_list matches;
>> + const char *match;
>> +
>> + va_start(matches, term);
>> + while (!res && (match = va_arg(matches, const char *)))
>> + res = !strcmp(term, match);
>> + va_end(matches);
>> +
>> + return res;
>> +}
>
> It might be safer to mark this function with LAST_ARG_MUST_BE_NULL,
> but because this is static to this function, it may not matter too
> much.  Just an observation, not a strong suggestion to change the
> patch.

Yes, I could do that.

>> +static int check_term_format(const char *term, const char *orig_term)
>> +{
>> + int res;
>> + char *new_term = xstrfmt("refs/bisect/%s", term);
>> +
>> + res = check_refname_format(new_term, 0);
>> + free(new_term);
>
> Yup, that looks much more straight-forward than using a one-time-use
> strbuf.

Thanks!

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

Re: [RFC/PATCH v11 03/13] bisect--helper: `write_terms` shell function in C

[Pranit Bauva]

Hey Junio,

On Tue, Aug 2, 2016 at 11:08 PM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>> +static int write_terms(const char *bad, const char *good)
>> +{
>> + FILE *fp;
>> + int res;
>> +
>> + if (!strcmp(bad, good))
>> + return error(_("please use two different terms"));
>> +
>> + if (check_term_format(bad, "bad") || check_term_format(good, "good"))
>> + return -1;
>> +
>> + fp = fopen(git_path_bisect_terms(), "w");
>> + if (!fp)
>> + return error_errno(_("could not open the file BISECT_TERMS"));
>> +
>> + res = fprintf(fp, "%s\n%s\n", bad, good);
>> + res |= fclose(fp);
>> + return (res < 0) ? -1 : 0;
>> +}
>
> If fprintf(3) were a function that returns 0 on success and negative
> on error (like fclose(3) is), the pattern to cascade the error
> return with "res |= another_call()" is appropriate, but the made me
> hiccup a bit while reading it.  It is not wrong per-se and it would
> certainly be making it worse if we did something silly like
>
> res = fprintf(...) < 0 ? -1 : 0;
> res |= fclose(fp);
>
> so I guess what you have is the most succinct way to do this.

I agree with your point and your suggested code is better!

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

Re: [PATCH v3 02/10] pkt-line: add direct_packet_write() and direct_packet_write_data()

[Jakub Narębski]

[This response might have been invalidated by v4]

W dniu 01.08.2016 o 14:00, Lars Schneider pisze:
>> On 30 Jul 2016, at 12:49, Jakub Narębski  wrote:
>> W dniu 30.07.2016 o 01:37, larsxschnei...@gmail.com pisze:
>>>
>>> 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?

Yes, I think it is more clear.  

The only thing that could be improved is to perhaps instead of using

  "then a proper pkt-line buffer with header and data section can be created"

it might be more clear to write

  "then a proper pkt-line buffer with data section and a place for pkt-line 
header"
 

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

Did you mean 

+   char hdr[PKTLINE_HEADER_LEN];
+   set_packet_header(hdr, sizeof(hdr) + size);

 
>>> +   !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 
> evaluates to true.
> I would think the second call is not evaluated, then?!

This is true both for || and for &&, as in C logical boolean operators
short-circuit.

> Should I make this more explicit with a if clause?

No need.

-- 
Jakub Narębski

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

Re: [PATCH v4 01/12] pkt-line: extract set_packet_header()

[Junio C Hamano]

larsxschnei...@gmail.com writes:

> From: Lars Schneider 
>
> set_packet_header() converts an integer to a 4 byte hex string. Make
> this function locally available so that other pkt-line functions can
> use it.

Didn't I say that this is a bad idea already in an earlier review?

The only reason why you want it, together with direct_packet_write()
(which I think is another bad idea), is because you use
packet_buf_write() to create a "" in a buf in the
usercode in step 11/12 like this:

+   packet_buf_write(, "command=%s\n", filter_type);
+   ret = !direct_packet_write(process->in, nbuf.buf, nbuf.len, 1);

which would be totally unnecessary if you just did strbuf_addf()
into nbuf and used packet_write() like everybody else does.

Puzzled.  Why are steps 01/12 and 02/12 an improvement?

--
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 6/7] submodule--helper: add remote-branch helper

[Stefan Beller]

On Wed, Aug 3, 2016 at 9:25 AM, Jeff King  wrote:
> On Thu, Jul 28, 2016 at 05:44:08PM -0700, Stefan Beller wrote:
>
>> +static const char *remote_submodule_branch(const char *path)
>> +{
>> + const struct submodule *sub;
>> + gitmodules_config();
>> + git_config(submodule_config, NULL);
>> +
>> + sub = submodule_from_path(null_sha1, path);
>> + if (!sub->branch)
>> + return "master";
>> +
>> + return sub->branch;
>> +}
>
> Coverity complains about "sub" being NULL here, and indeed, it seems
> like an easy segfault:
>
>   $ ./git submodule--helper remote-branch foo
>   Segmentation fault
>
> I guess this should return NULL in that case. But then this...

I thought about checking for (!sub && !sub->branch) to return "master";
However I disregarded that implementation as the submodule-config is
reliable for any correct input. Though we need to handle wrong input as
well. So how about:

  if (!sub)
die(_("A submodule doesn't exist at path %s"), path);
  if (!sub->branch)
return "master";

...

I think that explains best why we even check for !sub.


>
>> +static int resolve_remote_submodule_branch(int argc, const char **argv,
>> + const char *prefix)
>> +{
>> + struct strbuf sb = STRBUF_INIT;
>> + if (argc != 2)
>> + die("submodule--helper remote-branch takes exactly one 
>> arguments, got %d", argc);
>> +
>> + printf("%s", remote_submodule_branch(argv[1]));
>> + strbuf_release();
>> + return 0;
>> +}
>
> would need to handle the NULL return, as well. So maybe "master" or the
> empty string would be better. I haven't followed the topic closely
> enough to have an opinion.

Oh I see, this can turn into a discussion what the API for
remote_submodule_branch is. I think handling the NULL here and dying makes
more sense as then we can keep remote_submodule_branch pure to its
intended use case. (If in the far future we have all the submodule stuff in C,
we may want to call remote_submodule_branch when we already know
that the path is valid, so no need to check it in there. Also not dying in there
is more libified.)

Thanks for this review,
Stefan

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

On Wed, Aug 3, 2016 at 12:13 PM, Jeff King  wrote:
> On Mon, Aug 01, 2016 at 03:31:45PM -0700, Junio C Hamano wrote:
>
> I think in my head I rewrite any multiplication like "N of M" as having
> "N" as the smaller number. I.e., it is conceptually simpler to me to
> count five 30's, then 30 five's (even though I do not implement it in my
> head as a sequence of additions, of course; I'd probably do that
> particular case as "half of ten 30's").
>
> I have no idea if that's cultural or not, though.

Now, when you say "count five 30's", which one do you have
in mind? 5x30, or 30x5?

If you meant the former, I think that _is_ cultural. I am pretty
sure that I was taught in school(s) to read 5x30 as adding 5
thirty times.
--
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()

[Jakub Narębski]

[This response might have been invalidated by v4]

W dniu 01.08.2016 o 13:33, Lars Schneider pisze: 
>> On 30 Jul 2016, at 12:30, Jakub Narębski  wrote:

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

That's better, though I wonder if we need to start #defines at begining
of line.  But I think current proposal is O.K.


Either this (which has unnecessary larger scope)

  #define hex(a) (hexchar[(a) & 15])
  static void set_packet_header(char *buf, const int size)
  {
static char hexchar[] = "0123456789abcdef";

buf[0] = hex(size >> 12);
buf[1] = hex(size >> 8);
buf[2] = hex(size >> 4);
buf[3] = hex(size);
  }
  #undef hex

or this (which looks worse)

  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
  }

--
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 Wed, Aug 03, 2016 at 12:59:34PM -0700, Stefan Beller wrote:

> I think I'd find calloc intuitive as a typical textbook question, "I
> want to have space for "foos", which each cost 5 memory, go figure out
> how much I need and hand it back to me".

I think it is more a language question than a math one, though. Is it "5
instances, costing 30 each" or "30-cost instances, 5 times". They are
the same semantically (there is no "30 instances of a 5-cost thing",
which does not map to the problem space), but just with a different
order.

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

[Christian Couder]

On Wed, Aug 3, 2016 at 9:13 PM, Jeff King  wrote:
> On Mon, Aug 01, 2016 at 03:31:45PM -0700, Junio C Hamano wrote:
>
>> 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?
>
> I think in my head I rewrite any multiplication like "N of M" as having
> "N" as the smaller number. I.e., it is conceptually simpler to me to
> count five 30's, then 30 five's (even though I do not implement it in my
> head as a sequence of additions, of course; I'd probably do that
> particular case as "half of ten 30's").
>
> I have no idea if that's cultural or not, though. I'm pretty sure "half
> of ten 30's" was not taught in schools. All I remember of grade school
> multiplication is them insisting we write down all of our steps, no
> matter how trivial the problem would be to do in our heads. :)

Yeah, I would be tempted to write all the steps too like this:

"An apple costs 30 yen and I have 5 of them" means:

Cost(1 apple) = 30 cents
Cost(5 apples) = 5 * Cost(1 apple) = 5 * 30 cents = 150 cents

so it would be more "5x30=150" than "30x5" in this case for 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: [OT] USENIX paper on Git

[David Lang]

On Wed, 3 Aug 2016, Santiago Torres wrote:


So if you want to treat Git as a cryptographic end-to-end distribution
mechanism, then no, it fails horribly at that. But the state of the art
in source code distribution, no matter which system you use, is much
less advanced than that. People download tarballs, even ones with GPG
signatures, all the time without verifying their contents. Most packages
distribute a sha1sum or similar (sometimes even gpg-signed), but quite
often the source of authority is questionable.


Yes, this happens an awful lot of times. We did some work with python's
pypi last year, and we found out that less than 1% of people actually
downloaded the gpg signature for the package they are retrieving[1].



For example, consider somebody downloading a new package for the first
time. They don't know the author in any out-of-band way, so any
signatures are likely meaningless. They _might_ be depending on the
source domain for some security (and using some hierarchical PKI like
TLS+x.509 to be sure they're talking to that domain), but in your threat
model, even well-known hosts like FSF could be compromised internally.

So yes, I think the current state of affairs (especially open-source) is
that people download and run possibly-compromised code all the time. But
I'm not sure that lack of tool support is really the limiting factor. Or
that it has turned out to be all that big a problem in practice.


I couldn't agree more. I feel that OSS is slowly moving towards a more
cryptographically robust, trust-based way of doing things, which I find
pleasing.


It's too easy to look at this from purely a technical, cryptographic point of 
view and miss a very important point.


It may be very easy to see that this was signed by "cool-internet-name" but how 
can I tell if this is really Joe Blow the developer? and if it is, I still have 
no way of knowing if he's working for the NSA or not.


The lack of meaningful termination of the signatures to the real world is why so 
few people bother to check package signatures, etc.


David Lang
--
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()

[Stefan Beller]

On Wed, Aug 3, 2016 at 12:49 PM, Jeff King  wrote:
> On Wed, Aug 03, 2016 at 12:41:30PM -0700, Junio C Hamano wrote:
>
>> On Wed, Aug 3, 2016 at 12:13 PM, Jeff King  wrote:
>> > On Mon, Aug 01, 2016 at 03:31:45PM -0700, Junio C Hamano wrote:
>> >
>> > I think in my head I rewrite any multiplication like "N of M" as having
>> > "N" as the smaller number. I.e., it is conceptually simpler to me to
>> > count five 30's, then 30 five's (even though I do not implement it in my
>> > head as a sequence of additions, of course; I'd probably do that
>> > particular case as "half of ten 30's").
>> >
>> > I have no idea if that's cultural or not, though.

Well I think there is a difference between how you do the math in your head
and between the textbook question.

In textbook I would expect 5x30, because first we need to talk about the
object before the price of the object makes sense:
"I am interested in 5 apples, and each apple costs 30 yen, so I am paying
150 yen". Only that in Europe you would substitute the 30 by 0.84 Euros
(integer-> number with 2 values after comma, not quite a float).

When doing the math in your head you look for the easy tricks, i.e.
x5  = x10 /2 or such.

I think I'd find calloc intuitive as a typical textbook question, "I
want to have
space for "foos", which each cost 5 memory, go figure out how much I need
and hand it back to 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: Client exit whilst running pre-receive hook : commit accepted but no post-receive hook ran

[Junio C Hamano]

Jeff King  writes:

> I agree it would be a good property to have. I think it's hard to do
> atomically, though. Certainly we can wait to tell the other side "your
> push has been recorded" until after the hook is run. But we would
> already have updated the refs locally at that point (and we must -- that
> is part of the interface to the post-receive hooks, that the refs are
> already in place). So would we roll-back the ref update then? Even that
> suffers from power failures, etc.
>
> So I'm not sure if making it truly atomic is all the feasible.

As long as the requirement is that post- hook must see the updated
ref in place, I do not think it is feasible to give "the hook always
runs once" guarantee, without cooperation by other parts of the flow
(think: pulling the power at an arbitrary point in the process).

A receiving repository can implement it all in the userland, I would
think, though:

 * A pre-receive hook records the intention to update a ref (from
   what old commit to what new commit), and does not return until
   that record is securely in a database;

 * A post-receive hook checks the entry in the database above (it
   _must_ find one), and atomically does its thing and marks the
   entry "done";

 * A separate sweeper scans the database for entries not yet marked
   as "done", sees if the ref has been already updated, and
   atomically does its thing and marks the entry "done" (the same
   can be done as part of a post-receive for previously pushed
   commit that pre-receive recorded but did not manage to run
   post-receive before the power was pulled or the user did \C-c).

As you originally described, the non-atomicity is not new; as long
as we have necessary hooks in place on the git-core side for
repositories that want a stronger guarantee, I do not think there is
any more thing we need to do on this topic.  If we can narrow the
window in a non-intrusive way, that would be a good thing to do,
though.

> However,
> we could certainly make things more robust than they are now.

And this change may to the "narrowing the window in a non-intrusive
way" (I wonder if we also need to lift the post-update hook the same
way, though).

But that would still not guarantee "the hook always runs once".
What we have is "the hook runs at most once".

Thanks.

> The simplest thing may be to just bump the post-receive hook before the
> status report. That opens up the question of whether clients are
> actually waiting already for the post-receive to finish. Looking at the
> code in send-pack, it looks like the network is hooked up to the
> sideband demuxer thread, which will read until EOF on the network. So we
> are waiting either way for the post-receive to run. It doesn't really
> matter if it happens before or after the report to the client.
>
> So I _think_ something like this would work:
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 15c323a..91d01f0 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1767,9 +1767,9 @@ int cmd_receive_pack(int argc, const char **argv, const 
> char *prefix)
>   execute_commands(commands, unpack_status, );
>   if (pack_lockfile)
>   unlink_or_warn(pack_lockfile);
> + run_receive_hook(commands, "post-receive", 1);
>   if (report_status)
>   report(commands, unpack_status);
> - run_receive_hook(commands, "post-receive", 1);
>   run_update_post_hook(commands);
>   if (auto_gc) {
>   const char *argv_gc_auto[] = {
>
> but maybe there are other gotchas.
>
> -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:

> On Wed, Aug 03, 2016 at 12:41:30PM -0700, Junio C Hamano wrote:
>
>> On Wed, Aug 3, 2016 at 12:13 PM, Jeff King  wrote:
>> > On Mon, Aug 01, 2016 at 03:31:45PM -0700, Junio C Hamano wrote:
>> >
>> > I think in my head I rewrite any multiplication like "N of M" as having
>> > "N" as the smaller number. I.e., it is conceptually simpler to me to
>> > count five 30's, then 30 five's (even though I do not implement it in my
>> > head as a sequence of additions, of course; I'd probably do that
>> > particular case as "half of ten 30's").
>> >
>> > I have no idea if that's cultural or not, though.
>> 
>> Now, when you say "count five 30's", which one do you have
>> in mind? 5x30, or 30x5?
>> 
>> If you meant the former, I think that _is_ cultural. I am pretty
>> sure that I was taught in school(s) to read 5x30 as adding 5
>> thirty times.
>
> I think I would say "30x5" in that case. But I'm not sure where that
> comes from, and I'm not even 100% sure that I would say that (after
> thinking about it, it's hard for me to figure out what I would have done
> if I _hadn't_ just thought about it).

By the way, reading "30x5" as "count five 30's" disagrees with
calloc(nmemb=5, size=30), which wants to add 30-byte necessary for
each member 5 times to allocate 150 bytes.

Anyway, this tangent is long enough ;-)


--
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 Wed, Aug 03, 2016 at 12:41:30PM -0700, Junio C Hamano wrote:

> On Wed, Aug 3, 2016 at 12:13 PM, Jeff King  wrote:
> > On Mon, Aug 01, 2016 at 03:31:45PM -0700, Junio C Hamano wrote:
> >
> > I think in my head I rewrite any multiplication like "N of M" as having
> > "N" as the smaller number. I.e., it is conceptually simpler to me to
> > count five 30's, then 30 five's (even though I do not implement it in my
> > head as a sequence of additions, of course; I'd probably do that
> > particular case as "half of ten 30's").
> >
> > I have no idea if that's cultural or not, though.
> 
> Now, when you say "count five 30's", which one do you have
> in mind? 5x30, or 30x5?
> 
> If you meant the former, I think that _is_ cultural. I am pretty
> sure that I was taught in school(s) to read 5x30 as adding 5
> thirty times.

I think I would say "30x5" in that case. But I'm not sure where that
comes from, and I'm not even 100% sure that I would say that (after
thinking about it, it's hard for me to figure out what I would have done
if I _hadn't_ just thought about it).

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

Re: Client exit whilst running pre-receive hook : commit accepted but no post-receive hook ran

[Jeff King]

On Tue, Aug 02, 2016 at 12:01:57PM -0400, Stephen Morton wrote:

> Sounds tricky. I do think it's important, almost a 'data integrity' issue,
> that IF a commit is received, THEN the post-receive hook must be run. Too
> much mission-critical stuff is based on post-receive hooks.

I agree it would be a good property to have. I think it's hard to do
atomically, though. Certainly we can wait to tell the other side "your
push has been recorded" until after the hook is run. But we would
already have updated the refs locally at that point (and we must -- that
is part of the interface to the post-receive hooks, that the refs are
already in place). So would we roll-back the ref update then? Even that
suffers from power failures, etc.

So I'm not sure if making it truly atomic is all the feasible. However,
we could certainly make things more robust than they are now.

The simplest thing may be to just bump the post-receive hook before the
status report. That opens up the question of whether clients are
actually waiting already for the post-receive to finish. Looking at the
code in send-pack, it looks like the network is hooked up to the
sideband demuxer thread, which will read until EOF on the network. So we
are waiting either way for the post-receive to run. It doesn't really
matter if it happens before or after the report to the client.

So I _think_ something like this would work:

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 15c323a..91d01f0 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1767,9 +1767,9 @@ int cmd_receive_pack(int argc, const char **argv, const 
char *prefix)
execute_commands(commands, unpack_status, );
if (pack_lockfile)
unlink_or_warn(pack_lockfile);
+   run_receive_hook(commands, "post-receive", 1);
if (report_status)
report(commands, unpack_status);
-   run_receive_hook(commands, "post-receive", 1);
run_update_post_hook(commands);
if (auto_gc) {
const char *argv_gc_auto[] = {

but maybe there are other gotchas.

-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

[Richard Ipsum]

On Thu, Jul 28, 2016 at 11:40:55PM -0700, Josh Triplett wrote:
> 
> 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.
> 

One other nice thing I've noticed about this tool is the
way series behave like regular git branches: I specify the name
of the series and from then on all other commands act on that
series until told otherwise.

git-appraise looks as though it might also have this behaviour.
I think it's a nice way to do it, since you don't generally
perform more than one review simultaneously. So I may well
use this idea in git-candidate if it's okay. :)

I haven't found time to use the tool to do any serious review
yet, but I'll try and post some more feedback when I do.

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

[Junio C Hamano]

Junio C Hamano  writes:

> If you mean to tell the user "I won't describe it in detail, if you
> really want to know,
> go run blame yourself", spell it out like so. I was hoping that you
> can summarize
> in-line there to help the readers here.

Here is a proposed fixup.

 t/t7063-status-untracked-cache.sh | 16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/t/t7063-status-untracked-cache.sh 
b/t/t7063-status-untracked-cache.sh
index d31d080..e0a8228 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -4,12 +4,16 @@ test_description='test untracked cache'
 
 . ./test-lib.sh
 
-# On some filesystems (e.g. FreeBSD's ext2 and ufs) this and that
-# happens when we do blah, which forces the untracked cache code to
-# take the slow path.  A test that wants to make sure the fast path
-# works correctly should call this helper to make mtime of the
-# containing directory in sync with the reality after doing blah and
-# before checking the fast path behaviour
+# On some filesystems (e.g. FreeBSD's ext2 and ufs) directory mtime
+# is updated lazily after contents in the directory changes, which
+# forces the untracked cache code to take the slow path.  A test
+# that wants to make sure that the fast path works correctly should
+# call this helper to make mtime of the containing directory in sync
+# with the reality before checking the fast path behaviour.
+#
+# See <20160803174522.5571-1-pclo...@gmail.com> if you want to know
+# more.
+
 sync_mtime () {
find . -type d -ls >/dev/null
 }
--
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