Page content is wider than view window

[Brady Trainor]

If I am reading the git book or manual (https://git-scm.com/), and zoom
in, and/or have browser sized to a fraction of the screen, I cannot see
all the text, and have to horizontally scroll back and forth to read at
that zoom.

This may also be for smaller laptop screens, so those with larger
desktop monitors may not see this often.

Can site designer consider this in layout?

For example, I find it easier to read manuals at
https://gitirc.eu/git.html.

(This is unfortunately a very common problem on websites, including
github's help pages.)

Thank you,

--
Brady

Re: [PATCH 2/2] repack: repack promisor objects if -a or -A is set

[Junio C Hamano]

Jonathan Tan  writes:

>> each other [*1*], so listing them in the offset order (with made-up
>> pathname information to _force_ that objects that live close
>> together in the original pack are grouped together by getting
>> similar names) might give us a better than horrible deltification.
>> I dunno.
>
> Before I write the NEEDSWORK comment, just to clarify - do you mean
> better than horrible object locality? I think deltification still occurs
> because pack-objects sorts the input when doing the windowed
> deltification, for example:

But what to delta against what else is determined by the pathname
info, which is now lost by enumerating objects without tree/history
walking.  By giving phoney pathnames to objects while enumerating
them in offset order and giving similar pathnames to objects closer
to each other, I was hoping that better bases will likely to be in
the same window.  The order in which objects are prepared and fed to
try_delta() is "group by type, and then sort by path-hash (major
key) and size (descending order, used as minor key)", so that
the largest among similarly named blobs is stored as base and other
blobs with similar name try to become delta against that one.

>   git rev-parse HEAD:fetch-pack.c HEAD HEAD^ HEAD^^ \
>   HEAD^^^ v2.17.0:fetch-pack.c | git pack-objects --window=2 abc
>
> produces a packfile with a delta, as checked by `verify-pack -v`.

Is that interesting?  You can make the same argument that
fast-import produces a packfile with a delta.

It is known to produce horrible delta---its delta base selection
essentially is "you are giving me a new object?  let me see if it
deltas well with the object you gave me immediately before that (and
nothing else)".

Re: [RFC] submodule: munge paths to submodule git directories

[Junio C Hamano]

Brandon Williams  writes:

> Introduce a function "strbuf_submodule_gitdir()" which callers can use
> to build a path to a submodule's gitdir.  This allows for a single
> location where we can munge the submodule name (by url encoding it)
> before using it as part of a path.

I am not sure about the name with "strbuf_" prefix; it is as bad as
using hungarian notation for variable names.

There probably are some existing offenders, but it is merely an
implementation detail (or a function signature) that the returned
value is communicated using a strbuf (contrast it with things like
strbuf_add() that is _about_ doing something to a strbuf), and in
the longer term I prefer to see them lose "strbuf_" from their names
and optionally use the same number of bytes to describe what they do
more clearly.  For this particular case, "submodule" and "gitdir"
are sufficient to signal what the function is about, I think, so the
"optionally use..." is not necessary---instead we get a name that is
shorte to type and to remember.

> Using submodule names as is continues to be not such a good idea.  Maybe
> we could apply something like this to stop using them as is.  url
> encoding seems like the easiest approach, but I've also heard
> suggestions that would could use the SHA1 of the submodule name.

Being human readable is a good trait to keep when possible.  

When you have two submodules with vastly different names
(e.g. "hello" and "bye"), and for some reason you need to go in to
.gitmodules and manually fix their entries up, "hash of name" does
not help you avoid mistakes (hashing "hello" and hashing "helo"
would give a name as different as hashing "bye", so when you see
[module "hel$something"] in .gitmodules, you would know that entry
is not about the "bye" module, but "hello" module, even if you do
not remember exactly if the module you want to manipulate was called
"hello" or "helo").  The same discussion applies against UUID.

Can you do it?

[Ms CHIANG Lai Yuen JP]

I have a Businesss Proposal for you, get ack to me for more details.

Re: [PATCH 2/2] repack: repack promisor objects if -a or -A is set

[Jonathan Tan]

> Just a note (and a request-for-sanity-check) and not meant to be a
> request to update the code, but with a still-in-pu 4b757a40 ("Use
> remote_odb_get_direct() and has_remote_odb()", 2018-08-02) in
> flight, repository_format_partial_clone is now gone.
> 
> I've tentatively resolved the above to read like so:
> 
>   if (has_remote_odb())
>   repack_promisor_objects(_args, );
> 
> but I am not sure if it makes sense to always require odb helper to
> be present for any promisor.  As long as you do not have need to
> actually access these missing objects, you do not need any remote
> odb access at all, in which case requiring has_remote_odb() as a
> precondition to concatenate the promisor packs to coalesce them into
> one pack does not make sense---you only want to know if there are
> any .promisor packs.
> 
> In other words, I suspect that the world is not black (i.e. partial
> clone, which always has remote-odb) and white (i.e. full repository,
> without remote-odb).  4b757a40 makes it impossible to have a gray
> (i.e. partial clone, but no access to remote-odb), which I am not
> sure if it is a good thing.

Thanks for the heads-up. This makes me realize that even the current
precondition (repository_format_partial_clone) is not an exact one - I
should only be doing this if I know that there is at least one promisor
object (if not, in the rare case that a repo is configured to be partial
but does not have any promisor objects, repack will generate an empty
packfile). In the next reroll, I'll take care of this, which should
avoid this merge issue too.

Re: [RFC] submodule: munge paths to submodule git directories

[Jonathan Nieder]

Hi,

Brandon Williams wrote:

> Commit 0383bbb901 (submodule-config: verify submodule names as paths,
> 2018-04-30) introduced some checks to ensure that submodule names don't
> include directory traversal components (e.g. "../").
>
> This addresses the vulnerability identified in 0383bbb901 but the root
> cause is that we use submodule names to construct paths to the
> submodule's git directory.  What we really should do is munge the
> submodule name before using it to construct a path.
>
> Introduce a function "strbuf_submodule_gitdir()" which callers can use
> to build a path to a submodule's gitdir.  This allows for a single
> location where we can munge the submodule name (by url encoding it)
> before using it as part of a path.
>
> Signed-off-by: Brandon Williams 
> ---
> Using submodule names as is continues to be not such a good idea.  Maybe
> we could apply something like this to stop using them as is.  url
> encoding seems like the easiest approach, but I've also heard
> suggestions that would could use the SHA1 of the submodule name.
>
> Any thoughts?

I like this idea.  It avoids the security and complexity problems of
funny nested directories, while still making the submodule git dirs
easy to find.

The current behavior has been particularly a problem in practice when
submodule names are nested:

[submodule "a"]
url = https://www.example.com/a
path = a/1

[submodule "a/b"]
url = https://www.example.com/a/b
path = a/2

We don't enforce any constraint on submodule names to prevent that,
but it causes hard to diagnose errors at clone time:

fatal: not a git repository: superproject/a/1/../../.git/modules/a
Unable to fetch in submodule path 'a/1'
fatal: not a git repository: superproject/a/1/../../.git/modules/a
fatal: not a git repository: superproject/a/1/../../.git/modules/a
fatal: not a git repository: superproject/a/1/../../.git/modules/a
Fetched in submodule 'a/1', but it did not contain 
55ca6286e3e4f4fba5d0448333fa99fc5a404a73. Direct fetching of that commit failed.

because the fetch in .git/modules/a is interfered with by
.git/modules/a/b.

[...]
> --- a/submodule.c
> +++ b/submodule.c
[...]
> @@ -1933,9 +1938,29 @@ int submodule_to_gitdir(struct strbuf *buf, const char 
> *submodule)
>   goto cleanup;
>   }
>   strbuf_reset(buf);
> - strbuf_git_path(buf, "%s/%s", "modules", sub->name);
> + strbuf_submodule_gitdir(buf, the_repository, sub->name);
>   }
>  
>  cleanup:
>   return ret;
>  }
> +
> +void strbuf_submodule_gitdir(struct strbuf *buf, struct repository *r,
> +  const char *submodule_name)
> +{
> + int modules_len;

nit: size_t

> +
> + strbuf_git_common_path(buf, r, "modules/");
> + modules_len = buf->len;
> + strbuf_addstr(buf, submodule_name);
> +
> + /*
> +  * If the submodule gitdir already exists using the old location then
> +  * return that.
> +  */

nit: "old-fashioned location" or something.  Maybe the function could
use an API comment describing what's going on (that there are two
naming conventions and we try first the old, then the new).

Should we validate the submodule_name here when accessing following the old
convention?

> + if (!access(buf->buf, F_OK))
> + return;
> +
> + strbuf_setlen(buf, modules_len);
> + strbuf_addstr_urlencode(buf, submodule_name, 1);
> +}
[...]
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -932,7 +932,7 @@ test_expect_success 'recursive relative submodules stay 
> relative' '
>   cd clone2 &&
>   git submodule update --init --recursive &&
>   echo "gitdir: ../.git/modules/sub3" >./sub3/.git_expect &&
> - echo "gitdir: ../../../.git/modules/sub3/modules/dirdir/subsub" 
> >./sub3/dirdir/subsub/.git_expect
> + echo "gitdir: 
> ../../../.git/modules/sub3/modules/dirdir%2fsubsub" 
> >./sub3/dirdir/subsub/.git_expect
>   ) &&
>   test_cmp clone2/sub3/.git_expect clone2/sub3/.git &&
>   test_cmp clone2/sub3/dirdir/subsub/.git_expect 
> clone2/sub3/dirdir/subsub/.git

Sensible.

Can there be a test of the compatibility code as well?  (I mean a test
that manually sets up a submodule in .git/modules/dirdir/subsub and
ensures that it gets reused.)

I'll apply this, experiment with it, and report back.  Thanks for
writing it.

Sincerely,
Jonathan

Re: [PATCH 2/2] repack: repack promisor objects if -a or -A is set

[Jonathan Tan]

> for_each_object_in_pack() is a fine way to make sure that you list
> everythning in a pack, but I suspect it is a horrible way to feed a
> list of objects to pack-objects, as it goes by the .idx order, which
> is by definition a way to enumerate objects in a randomly useless
> order.

That's true.

> Do we already have an access to the in-core reverse index for the
> pack at this point in the code?

As far as I can tell, no. These patches construct the list of promisor
objects in repack.c (which does not use the revindex at all), to be
processed by pack-objects in a different process (which does use the
revindex in reuse-delta mode, which is the default). I guess that we
could have access to the revindex if we were to libify pack-objects and
run it in the same process as repack.c or if we were to add a special
mode to pack-objects that reads for itself the list of all the promisor
objects.

> If so, we can enumerate the objects
> in the offset order without incurring a lot of cost (building the
> in-core revindex is the more expensive part).  When writing a pack,
> we try to make sure that related objects are written out close to
> each other [*1*], so listing them in the offset order (with made-up
> pathname information to _force_ that objects that live close
> together in the original pack are grouped together by getting
> similar names) might give us a better than horrible deltification.
> I dunno.

Before I write the NEEDSWORK comment, just to clarify - do you mean
better than horrible object locality? I think deltification still occurs
because pack-objects sorts the input when doing the windowed
deltification, for example:

  git rev-parse HEAD:fetch-pack.c HEAD HEAD^ HEAD^^ \
HEAD^^^ v2.17.0:fetch-pack.c | git pack-objects --window=2 abc

produces a packfile with a delta, as checked by `verify-pack -v`.

>   Side note *1*: "related" has two axis, and one is relevant
>   for better deltification, while the other is not useful.
>   The one I have in mind here is that we write set of blobs
>   that belong to the same "delta family" together.

As far as I can see, they do not need to be adjacent in the packfile to
have one be a delta against the other.

> I do not think such a "make it a bit better than horrible" is necessary
> in an initial version, but it deserves an in-code NEEDSWORK comment
> to remind us that we need to measure and experiment.

OK, I'll do this in the next reroll.

[RFC] submodule: munge paths to submodule git directories

[Brandon Williams]

Commit 0383bbb901 (submodule-config: verify submodule names as paths,
2018-04-30) introduced some checks to ensure that submodule names don't
include directory traversal components (e.g. "../").

This addresses the vulnerability identified in 0383bbb901 but the root
cause is that we use submodule names to construct paths to the
submodule's git directory.  What we really should do is munge the
submodule name before using it to construct a path.

Introduce a function "strbuf_submodule_gitdir()" which callers can use
to build a path to a submodule's gitdir.  This allows for a single
location where we can munge the submodule name (by url encoding it)
before using it as part of a path.

Signed-off-by: Brandon Williams 
---

Using submodule names as is continues to be not such a good idea.  Maybe
we could apply something like this to stop using them as is.  url
encoding seems like the easiest approach, but I've also heard
suggestions that would could use the SHA1 of the submodule name.

Any thoughts?

 builtin/submodule--helper.c  | 10 --
 dir.c|  2 +-
 repository.c |  3 +-
 submodule.c  | 57 +++-
 submodule.h  |  3 ++
 t/t7400-submodule-basic.sh   |  2 +-
 t/t7406-submodule-update.sh  | 21 
 t/t7410-submodule-checkout-to.sh |  6 ++--
 8 files changed, 65 insertions(+), 39 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index bd250ca216..37b7353167 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1122,18 +1122,24 @@ static int add_possible_reference_from_superproject(
 * standard layout with .git/(modules/)+/objects
 */
if (ends_with(alt->path, "/objects")) {
+   struct repository alternate;
char *sm_alternate;
struct strbuf sb = STRBUF_INIT;
struct strbuf err = STRBUF_INIT;
strbuf_add(, alt->path, strlen(alt->path) - 
strlen("objects"));
 
+   repo_init(, sb.buf, NULL);
+
/*
 * We need to end the new path with '/' to mark it as a dir,
 * otherwise a submodule name containing '/' will be broken
 * as the last part of a missing submodule reference would
 * be taken as a file name.
 */
-   strbuf_addf(, "modules/%s/", sas->submodule_name);
+   strbuf_reset();
+   strbuf_submodule_gitdir(, , sas->submodule_name);
+   strbuf_addch(, '/');
+   repo_clear();
 
sm_alternate = compute_alternate_path(sb.buf, );
if (sm_alternate) {
@@ -1246,7 +1252,7 @@ static int module_clone(int argc, const char **argv, 
const char *prefix)
usage_with_options(git_submodule_helper_usage,
   module_clone_options);
 
-   strbuf_addf(, "%s/modules/%s", get_git_dir(), name);
+   strbuf_submodule_gitdir(, the_repository, name);
sm_gitdir = absolute_pathdup(sb.buf);
strbuf_reset();
 
diff --git a/dir.c b/dir.c
index fe9bf58e4c..3463a5e0a5 100644
--- a/dir.c
+++ b/dir.c
@@ -3052,7 +3052,7 @@ static void connect_wt_gitdir_in_nested(const char 
*sub_worktree,
strbuf_reset(_wt);
strbuf_reset(_gd);
strbuf_addf(_wt, "%s/%s", sub_worktree, sub->path);
-   strbuf_addf(_gd, "%s/modules/%s", sub_gitdir, sub->name);
+   strbuf_submodule_gitdir(_gd, , sub->name);
 
connect_work_tree_and_git_dir(sub_wt.buf, sub_gd.buf, 1);
}
diff --git a/repository.c b/repository.c
index 02fe884603..15fabbd08d 100644
--- a/repository.c
+++ b/repository.c
@@ -194,8 +194,7 @@ int repo_submodule_init(struct repository *submodule,
 * submodule would not have a worktree.
 */
strbuf_reset();
-   strbuf_repo_git_path(, superproject,
-"modules/%s", sub->name);
+   strbuf_submodule_gitdir(, superproject, sub->name);
 
if (repo_init(submodule, gitdir.buf, NULL)) {
ret = -1;
diff --git a/submodule.c b/submodule.c
index 939d6870ec..1d571845e8 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1625,20 +1625,22 @@ int submodule_move_head(const char *path,
absorb_git_dir_into_superproject("", path,
ABSORB_GITDIR_RECURSE_SUBMODULES);
} else {
-   char *gitdir = xstrfmt("%s/modules/%s",
-   get_git_common_dir(), sub->name);
-   connect_work_tree_and_git_dir(path, gitdir, 0);
-   free(gitdir);
+   struct strbuf gitdir = STRBUF_INIT;
+   strbuf_submodule_gitdir(, 

Re: [PATCH 2/2] repack: repack promisor objects if -a or -A is set

[Junio C Hamano]

Jonathan Tan  writes:

> @@ -293,6 +346,9 @@ int cmd_repack(int argc, const char **argv, const char 
> *prefix)
>   if (pack_everything & ALL_INTO_ONE) {
>   get_non_kept_pack_filenames(_packs, _pack_list);
>  
> + if (repository_format_partial_clone)
> + repack_promisor_objects(_args, );
> +
>   if (existing_packs.nr && delete_redundant) {
>   if (unpack_unreachable) {
>   argv_array_pushf(,

Just a note (and a request-for-sanity-check) and not meant to be a
request to update the code, but with a still-in-pu 4b757a40 ("Use
remote_odb_get_direct() and has_remote_odb()", 2018-08-02) in
flight, repository_format_partial_clone is now gone.

I've tentatively resolved the above to read like so:

if (has_remote_odb())
repack_promisor_objects(_args, );

but I am not sure if it makes sense to always require odb helper to
be present for any promisor.  As long as you do not have need to
actually access these missing objects, you do not need any remote
odb access at all, in which case requiring has_remote_odb() as a
precondition to concatenate the promisor packs to coalesce them into
one pack does not make sense---you only want to know if there are
any .promisor packs.

In other words, I suspect that the world is not black (i.e. partial
clone, which always has remote-odb) and white (i.e. full repository,
without remote-odb).  4b757a40 makes it impossible to have a gray
(i.e. partial clone, but no access to remote-odb), which I am not
sure if it is a good thing.

Re: [PATCH 1/2] git-instaweb: support Fedora/Red Hat apache module path

[Junio C Hamano]

Junio C Hamano  writes:

>   if test -z "$module_path"
>   then
>   for candidate in \
>   /etc/httpd \
>   /usr/lib/apache2 \
>   /usr/lib/httpd \

I obviously missed semicolon here...

>   do
>   if test -d "$candidate/modules"
>   then
>   module_path="$candidate/modules"
>   break
>   fi

One more thing to note is that the fourth candidate might not end
with "/modules" and force us to update these existing three to have
"/modules" at the end and lose appending "/modules" from these two
lines to compensate.  That is sort of deliberate (i.e. as long as we
can share "/modules" as a common substring at the end, it is OK to
take advantage of that).

>   done
>   fi
>
> is when you go from 2 to 3.  Two points to note are:
>
>  - It would be easier to add the fourth one this way
>
>  - The explicit "break" makes it clear that the paths are listed in
>decreasing order of precedence (i.e. /etc/httpd if exists makes
>/usr/lib/httpd ignored even if the latter exists); the original
>"test -d X && M=X ; test -d Y && M=Y" gives higher precedence to
>the later items but readers need to wonder if it is intended or
>the code is simply being sloppy.
>
> Hope this helps.

Re: [PATCH 4/4] line-log: convert an assertion to a full BUG() call

[Eric Sunshine]

On Tue, Aug 7, 2018 at 5:09 AM Eric Sunshine  wrote:
> On Mon, Aug 6, 2018 at 9:15 AM Johannes Schindelin
>  wrote:
> > I think Andrei's assessment is wrong. The code could not test for that
> > earlier, as it did allow ranges to become "abutting" in the process, by
> > failing to merge them. So the invariant you talked about is more of an
> > invariant for the initial state.
>
> My understanding is that range_set_append() is intended to be strict
> by not allowing addition of a range which abuts an existing range
> (although, of course, the assert() checks only the last range, so it's
> not full-proof).

Ignore my parenthetical comment. It is clearly wrong.

Looking at this again, I see that there is some confusion. The comment
in line-log.h says:

/* New range must begin at or after end of last added range */
   void range_set_append(struct range_set *, long start, long end);

However, that comment was added by me in c0babbe695 (range-set:
publish API for re-use by git-blame -L, 2013-08-06) -- five years and
one day ago -- and it appears to be based upon a direct interpretation
of the condition in the assert(), including its off-by-one error.

*But*, one of the invariants of range-set is that the ranges must
_not_ abut one another, so the "at or after" is wrong; it should say
instead something like "after, and not touching, the end of the last
added range".

That invariant about having a gap between ranges is enforced
deliberately by range_set_check_invariants(). It's also signaled
implicitly by the fact that no callers of range_set_append() ever
invoke sort_and_merge_range_set() after calling range_set_append().

Re: [RFC PATCH] line-log: clarify [a,b) notation for ranges

[Eric Sunshine]

On Tue, Aug 7, 2018 at 9:54 AM Andrei Rybak  wrote:
> line-log.[ch] use left-closed, right-open interval logic. Change comment
> and debug output to square brackets+parentheses notation to help
> developers avoid off-by-one errors.
> ---

This seems sensible. There might be some reviewers who suggest
different notation since "[...)" is not universal (see [1]), but I
think this is fine.

You'll want to add your sign-off, of course, when taking this out of RFC.

[1]: 
https://en.wikipedia.org/wiki/Interval_(mathematics)#Notations_for_intervals

> line-log.c also uses ASCII graphics |---| in some comments, like:
>
> /*
>  * a: |---
>  * b: --|
>  */
>
> But I'm not sure if replacing them with
>
> /*
>  * a: [---
>  * b: --)
>  */
>
> will be helpful.

Those comments seem to use horizontal ruling to make it clear where
one range ends and another begins, so they already do a good job
conveying what they represent. Changing them to use "["" and ")" might
make them less clear.

> Another possibility is to update comment for
> range_set_append_unsafe to read
>
>   /* tack on a _new_ range [a,b) _at the end_ */
>   void range_set_append_unsafe(struct range_set *rs, long a, long b)

It shouldn't hurt (though the existing "_at the end_" is rather
superfluous since "append" in the name says the that already).

Re: [PATCH 1/2] git-instaweb: support Fedora/Red Hat apache module path

[Junio C Hamano]

Sebastian Kisela  writes:

> On Fedora-derived systems, the apache httpd package installs modules
> under /usr/lib{,64}/httpd/modules, depending on whether the system is
> 32- or 64-bit.  A symlink from /etc/httpd/modules is created which
> points to the proper module path.  Use it to support apache on Fedora,
> CentOS, and Red Hat systems.
>
> Written with assistance of Todd Zullinger 
>
> Signed-off-by: Sebastian Kisela 
> ---
>  git-instaweb.sh | 2 ++
>  1 file changed, 2 insertions(+)

Thanks for a patch, and welcome to the Git development community.

> diff --git a/git-instaweb.sh b/git-instaweb.sh
> index 47e38f34c..e42e58528 100755
> --- a/git-instaweb.sh
> +++ b/git-instaweb.sh
> @@ -332,6 +332,8 @@ apache2_conf () {
>   module_path="/usr/lib/httpd/modules"
>   test -d "/usr/lib/apache2/modules" &&
>   module_path="/usr/lib/apache2/modules"
> + test -d "/etc/httpd/modules" &&
> + module_path="/etc/httpd/modules"

The original already has the same issue with two entries but this
patch makes it even worse by adding yet another one.  The longer the
code follows a bad pattern, the more it encourages future developers
to follow the same bad pattern, and usually the best time to do a
clean up like the following

if test -z "$module_path"
then
for candidate in \
/etc/httpd \
/usr/lib/apache2 \
/usr/lib/httpd \
do
if test -d "$candidate/modules"
then
module_path="$candidate/modules"
break
fi
done
fi

is when you go from 2 to 3.  Two points to note are:

 - It would be easier to add the fourth one this way

 - The explicit "break" makes it clear that the paths are listed in
   decreasing order of precedence (i.e. /etc/httpd if exists makes
   /usr/lib/httpd ignored even if the latter exists); the original
   "test -d X && M=X ; test -d Y && M=Y" gives higher precedence to
   the later items but readers need to wonder if it is intended or
   the code is simply being sloppy.

Hope this helps.

Re: [PATCH v2 0/4] fix "rebase -i --root" corrupting root commit

[Junio C Hamano]

Eric Sunshine  writes:

> On Mon, Aug 6, 2018 at 9:20 PM Hilco Wijbenga  
> wrote:
>> But your suggestion did make me think about what behaviour I would
>> like to see, exactly. I like that Git removes commits that no longer
>> serve any purpose (because I've included their changes in earlier
>> commits). So I would not want to keep commits that become empty during
>> the rebase. What I would like to see is that commits that _start out_
>> as empty, are retained. Would such behaviour make sense? Or would that
>> be considered surprising behaviour?
>
> I, personally, have no opinion since I don't use empty commits.
> Perhaps someone more experienced and more long-sighted will chime in.

0661e49a ("git-rebase.txt: document behavioral differences between
modes", 2018-06-27) added the following.  In short, "rebase -i"
should already behave that way.

+ * empty commits:
+
+am-based rebase will drop any "empty" commits, whether the
+commit started empty (had no changes relative to its parent to
+start with) or ended empty (all changes were already applied
+upstream in other commits).
+
+merge-based rebase does the same.
+
+interactive-based rebase will by default drop commits that
+started empty and halt if it hits a commit that ended up empty.
+The `--keep-empty` option exists for interactive rebases to allow
+it to keep commits that started empty.

Re: [PATCH v7 0/1] sideband: highlight keywords in remote sideband output

[Junio C Hamano]

Han-Wen Nienhuys  writes:

> Fix nits; remove debug printf.
>
> Han-Wen Nienhuys (1):
>   sideband: highlight keywords in remote sideband output
>
>  Documentation/config.txt|  12 +++
>  help.c  |   1 +
>  help.h  |   1 +
>  sideband.c  | 125 ++--
>  t/t5409-colorize-remote-messages.sh |  87 +++
>  5 files changed, 217 insertions(+), 9 deletions(-)
>  create mode 100755 t/t5409-colorize-remote-messages.sh
>
> --
> 2.18.0.597.ga71716f1ad-goog

I'll squash in the following while queuing for



Thanks for sticking to the topic.  

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 33bc1a3def..9a38dd2cbb 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1266,7 +1266,7 @@ color.push.error::
 color.remote::
If set, keywords at the start of the line are highlighted. The
keywords are "error", "warning", "hint" and "success", and are
-   matched case-insensitively. Maybe set to `always`, `false` (or
+   matched case-insensitively. May be set to `always`, `false` (or
`never`) or `auto` (or `true`). If unset, then the value of
`color.ui` is used (`auto` by default).
 

Re: [PATCH 2/2] repack: repack promisor objects if -a or -A is set

[Junio C Hamano]

Jonathan Tan  writes:

> +static int write_oid(const struct object_id *oid, struct packed_git *pack,
> +  uint32_t pos, void *data)
> +{
> + int fd = *(int *)data;
> +
> + xwrite(fd, oid_to_hex(oid), GIT_SHA1_HEXSZ);
> + xwrite(fd, "\n", 1);
> + return 0;
> +}
> +
> +static void repack_promisor_objects(const struct packed_objects_args *args,
> + struct string_list *names)
> +{
> + struct child_process cmd = CHILD_PROCESS_INIT;
> + FILE *out;
> + struct strbuf line = STRBUF_INIT;
> +
> + prepare_packed_objects(, args);
> + cmd.in = -1;
> +
> + if (start_command())
> + die("Could not start pack-objects to repack promisor objects");
> +
> + for_each_packed_object(write_oid, ,
> +FOR_EACH_OBJECT_PROMISOR_ONLY);
> + close(cmd.in);

for_each_object_in_pack() is a fine way to make sure that you list
everythning in a pack, but I suspect it is a horrible way to feed a
list of objects to pack-objects, as it goes by the .idx order, which
is by definition a way to enumerate objects in a randomly useless
order.

Do we already have an access to the in-core reverse index for the
pack at this point in the code?  If so, we can enumerate the objects
in the offset order without incurring a lot of cost (building the
in-core revindex is the more expensive part).  When writing a pack,
we try to make sure that related objects are written out close to
each other [*1*], so listing them in the offset order (with made-up
pathname information to _force_ that objects that live close
together in the original pack are grouped together by getting
similar names) might give us a better than horrible deltification.
I dunno.

Side note *1*: "related" has two axis, and one is relevant
for better deltification, while the other is not useful.
The one I have in mind here is that we write set of blobs
that belong to the same "delta family" together.

I do not think such a "make it a bit better than horrible" is necessary
in an initial version, but it deserves an in-code NEEDSWORK comment
to remind us that we need to measure and experiment.

Thanks.

[PATCH 0/2] Repacking of promisor packfiles

[Jonathan Tan]

These patches teach Git to also repack promisor packfiles upon GC, which
reduces one of the pain points of current partial clone usage (many
promisor packfiles in the objects/pack/ directory, generated upon each
fetch).

In the t/ tests, I strived to verify that repack doesn't accidentally
delete any objects. Let me know if you can think of better ways to do
that.

Jonathan Tan (2):
  repack: refactor setup of pack-objects cmd
  repack: repack promisor objects if -a or -A is set

 Documentation/git-repack.txt |   5 ++
 builtin/repack.c | 163 ---
 t/t0410-partial-clone.sh |  71 ---
 3 files changed, 180 insertions(+), 59 deletions(-)

-- 
2.18.0.597.ga71716f1ad-goog

[PATCH 2/2] repack: repack promisor objects if -a or -A is set

[Jonathan Tan]

Currently, repack does not touch promisor packfiles at all, potentially
causing the performance of repositories that have many such packfiles to
drop. Therefore, repack all promisor objects if invoked with -a or -A.

This is done by an additional invocation of pack-objects on all promisor
objects individually given, which takes care of deduplication and allows
the resulting packfiles to respect flags such as --max-pack-size.

Signed-off-by: Jonathan Tan 
---
 Documentation/git-repack.txt |  5 +++
 builtin/repack.c | 64 ++--
 t/t0410-partial-clone.sh | 71 ++--
 3 files changed, 125 insertions(+), 15 deletions(-)

diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt
index d90e7907f..d05625096 100644
--- a/Documentation/git-repack.txt
+++ b/Documentation/git-repack.txt
@@ -40,6 +40,11 @@ OPTIONS
 Note that users fetching over dumb protocols will have to fetch the
 whole new pack in order to get any contained object, no matter how many
 other objects in that pack they already have locally.
++
+Promisor packfiles are repacked separately: if there are packfiles that
+have an associated ".promisor" file, these packfiles will be repacked
+into another separate pack, and an empty ".promisor" file corresponding
+to the new separate pack will be written.
 
 -A::
Same as `-a`, unless `-d` is used.  Then any unreachable
diff --git a/builtin/repack.c b/builtin/repack.c
index ca695fa10..a78d2f0aa 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -8,6 +8,7 @@
 #include "strbuf.h"
 #include "string-list.h"
 #include "argv-array.h"
+#include "packfile.h"
 
 static int delta_base_offset = 1;
 static int pack_kept_objects = -1;
@@ -83,7 +84,7 @@ static void remove_pack_on_signal(int signo)
 
 /*
  * Adds all packs hex strings to the fname list, which do not
- * have a corresponding .keep or .promisor file. These packs are not to
+ * have a corresponding .keep file. These packs are not to
  * be kept if we are going to pack everything into one file.
  */
 static void get_non_kept_pack_filenames(struct string_list *fname_list,
@@ -111,8 +112,7 @@ static void get_non_kept_pack_filenames(struct string_list 
*fname_list,
 
fname = xmemdupz(e->d_name, len);
 
-   if (!file_exists(mkpath("%s/%s.keep", packdir, fname)) &&
-   !file_exists(mkpath("%s/%s.promisor", packdir, fname)))
+   if (!file_exists(mkpath("%s/%s.keep", packdir, fname)))
string_list_append_nodup(fname_list, fname);
else
free(fname);
@@ -122,7 +122,7 @@ static void get_non_kept_pack_filenames(struct string_list 
*fname_list,
 
 static void remove_redundant_pack(const char *dir_name, const char *base_name)
 {
-   const char *exts[] = {".pack", ".idx", ".keep", ".bitmap"};
+   const char *exts[] = {".pack", ".idx", ".keep", ".bitmap", ".promisor"};
int i;
struct strbuf buf = STRBUF_INIT;
size_t plen;
@@ -179,6 +179,58 @@ static void prepare_packed_objects(struct child_process 
*cmd,
cmd->out = -1;
 }
 
+static int write_oid(const struct object_id *oid, struct packed_git *pack,
+uint32_t pos, void *data)
+{
+   int fd = *(int *)data;
+
+   xwrite(fd, oid_to_hex(oid), GIT_SHA1_HEXSZ);
+   xwrite(fd, "\n", 1);
+   return 0;
+}
+
+static void repack_promisor_objects(const struct packed_objects_args *args,
+   struct string_list *names)
+{
+   struct child_process cmd = CHILD_PROCESS_INIT;
+   FILE *out;
+   struct strbuf line = STRBUF_INIT;
+
+   prepare_packed_objects(, args);
+   cmd.in = -1;
+
+   if (start_command())
+   die("Could not start pack-objects to repack promisor objects");
+
+   for_each_packed_object(write_oid, ,
+  FOR_EACH_OBJECT_PROMISOR_ONLY);
+   close(cmd.in);
+
+   out = xfdopen(cmd.out, "r");
+   while (strbuf_getline_lf(, out) != EOF) {
+   char *promisor_name;
+   int fd;
+   if (line.len != 40)
+   die("repack: Expecting 40 character sha1 lines only 
from pack-objects.");
+   string_list_append(names, line.buf);
+
+   /*
+* pack-objects creates the .pack and .idx files, but not the
+* .promisor file. Create the .promisor file, which is empty.
+*/
+   promisor_name = mkpathdup("%s-%s.promisor", packtmp,
+ line.buf);
+   fd = open(promisor_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
+   if (fd < 0)
+   die_errno("unable to create '%s'", promisor_name);
+   close(fd);
+   free(promisor_name);
+   }
+   fclose(out);
+   if (finish_command())
+   die("Could not finish 

[PATCH 1/2] repack: refactor setup of pack-objects cmd

[Jonathan Tan]

A subsequent patch will teach repack to run pack-objects with some same
and some different arguments if repacking of promisor objects is
required. Refactor the setup of the pack-objects cmd so that setting up
the arguments common to both is done in a function.

Signed-off-by: Jonathan Tan 
---
 builtin/repack.c | 99 +++-
 1 file changed, 55 insertions(+), 44 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 6c636e159..ca695fa10 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -138,6 +138,47 @@ static void remove_redundant_pack(const char *dir_name, 
const char *base_name)
strbuf_release();
 }
 
+struct packed_objects_args {
+   const char *window;
+   const char *window_memory;
+   const char *depth;
+   const char *threads;
+   const char *max_pack_size;
+   int no_reuse_delta;
+   int no_reuse_object;
+   int quiet;
+   int local;
+};
+
+static void prepare_packed_objects(struct child_process *cmd,
+  const struct packed_objects_args *args)
+{
+   argv_array_push(>args, "pack-objects");
+   if (args->window)
+   argv_array_pushf(>args, "--window=%s", args->window);
+   if (args->window_memory)
+   argv_array_pushf(>args, "--window-memory=%s", 
args->window_memory);
+   if (args->depth)
+   argv_array_pushf(>args, "--depth=%s", args->depth);
+   if (args->threads)
+   argv_array_pushf(>args, "--threads=%s", args->threads);
+   if (args->max_pack_size)
+   argv_array_pushf(>args, "--max-pack-size=%s", 
args->max_pack_size);
+   if (args->no_reuse_delta)
+   argv_array_pushf(>args, "--no-reuse-delta");
+   if (args->no_reuse_object)
+   argv_array_pushf(>args, "--no-reuse-object");
+   if (args->local)
+   argv_array_push(>args,  "--local");
+   if (args->quiet)
+   argv_array_push(>args,  "--quiet");
+   if (delta_base_offset)
+   argv_array_push(>args,  "--delta-base-offset");
+   argv_array_push(>args, packtmp);
+   cmd->git_cmd = 1;
+   cmd->out = -1;
+}
+
 #define ALL_INTO_ONE 1
 #define LOOSEN_UNREACHABLE 2
 
@@ -165,15 +206,9 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
int delete_redundant = 0;
const char *unpack_unreachable = NULL;
int keep_unreachable = 0;
-   const char *window = NULL, *window_memory = NULL;
-   const char *depth = NULL;
-   const char *threads = NULL;
-   const char *max_pack_size = NULL;
struct string_list keep_pack_list = STRING_LIST_INIT_NODUP;
-   int no_reuse_delta = 0, no_reuse_object = 0;
int no_update_server_info = 0;
-   int quiet = 0;
-   int local = 0;
+   struct packed_objects_args po_args = {NULL};
 
struct option builtin_repack_options[] = {
OPT_BIT('a', NULL, _everything,
@@ -183,14 +218,14 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
   LOOSEN_UNREACHABLE | ALL_INTO_ONE),
OPT_BOOL('d', NULL, _redundant,
N_("remove redundant packs, and run 
git-prune-packed")),
-   OPT_BOOL('f', NULL, _reuse_delta,
+   OPT_BOOL('f', NULL, _args.no_reuse_delta,
N_("pass --no-reuse-delta to 
git-pack-objects")),
-   OPT_BOOL('F', NULL, _reuse_object,
+   OPT_BOOL('F', NULL, _args.no_reuse_object,
N_("pass --no-reuse-object to 
git-pack-objects")),
OPT_BOOL('n', NULL, _update_server_info,
N_("do not run git-update-server-info")),
-   OPT__QUIET(, N_("be quiet")),
-   OPT_BOOL('l', "local", ,
+   OPT__QUIET(_args.quiet, N_("be quiet")),
+   OPT_BOOL('l', "local", _args.local,
N_("pass --local to git-pack-objects")),
OPT_BOOL('b', "write-bitmap-index", _bitmaps,
N_("write bitmap index")),
@@ -198,15 +233,15 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
N_("with -A, do not loosen objects older than 
this")),
OPT_BOOL('k', "keep-unreachable", _unreachable,
N_("with -a, repack unreachable objects")),
-   OPT_STRING(0, "window", , N_("n"),
+   OPT_STRING(0, "window", _args.window, N_("n"),
N_("size of the window used for delta 
compression")),
-   OPT_STRING(0, "window-memory", _memory, N_("bytes"),
+   OPT_STRING(0, "window-memory", _args.window_memory, 
N_("bytes"),
N_("same as the above, but limit memory size 
instead of entries count")),
-   

Re: [PATCH v2] clone: report duplicate entries on case-insensitive filesystems

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

>  One nice thing about this is we don't need platform specific code for
>  detecting the duplicate entries. I think ce_match_stat() works even
>  on Windows. And it's now equally expensive on all platforms :D

ce_match_stat() may not be a very good measure to see if two paths
refer to the same file, though.  After a fresh checkout, I would not
be surprised if two completely unrelated paths have the same size
and have same mtime/ctime.  In its original use case, i.e. "I have
one specific path in mind and took a snapshot of its metadata
earlier.  Is it still the same, or has it been touched?", that may
be sufficient to detect that the path has been _modified_, but
without reliable inum, it may be a poor measure to say two paths
refer to the same.

>  builtin/clone.c |  1 +
>  cache.h |  2 ++
>  entry.c | 44 
>  unpack-trees.c  | 23 +++
>  unpack-trees.h  |  1 +
>  5 files changed, 71 insertions(+)

Having said that, it is pleasing to see that this can be achieved
with so little additional code.

> +static void mark_duplicate_entries(const struct checkout *state,
> +struct cache_entry *ce, struct stat *st)
> +{
> + int i;
> + int *count = state->nr_duplicates;
> +
> + if (!count)
> + BUG("state->nr_duplicates must not be NULL");
> +
> + ce->ce_flags |= CE_MATCHED;
> + (*count)++;
> +
> + if (!state->refresh_cache)
> + BUG("We need this to narrow down the set of updated entries");
> +
> + for (i = 0; i < state->istate->cache_nr; i++) {
> + struct cache_entry *dup = state->istate->cache[i];
> +
> + /*
> +  * This entry has not been checked out yet, otherwise
> +  * its stat info must have been updated. And since we
> +  * check out from top to bottom, the rest is guaranteed
> +  * not checked out. Stop now.
> +  */
> + if (!ce_uptodate(dup))
> + break;
> +
> + if (dup->ce_flags & CE_MATCHED)
> + continue;
> +
> + if (ce_match_stat(dup, st,
> +   CE_MATCH_IGNORE_VALID |
> +   CE_MATCH_IGNORE_SKIP_WORKTREE))
> + continue;
> +
> + dup->ce_flags |= CE_MATCHED;
> + (*count)++;
> + break;
> + }
> +}
> +

Hmph.  If there is only one true collision, then all its aliases
will be marked with CE_MATCHED bit every time the second and the
subsequent alias is checked out (as the caller calls this function
when it noticed that something already is at the path ce wants to
go).  But if there are two sets of colliding paths, because there is
only one bit used, we do not group the paths into these two sets and
report, e.g. "blue.txt, BLUE.txt and BLUE.TXT collide.  red.txt and
RED.txt also collide."  I am not sure if computing that is too much
work for too little gain, but because this is in an error codepath,
it may be worth doing.  I dunno.

> +
> + if (o->clone && state.nr_duplicates) {
> + warning(_("the following paths in this repository only differ 
> in case\n"
> +   "from another path and will cause problems because 
> you have cloned\n"
> +   "it on an case-insensitive filesytem:\n"));

With the new approach, we no longer preemptively detect that the
project will be harmed by a case smashing filesystems before it
happens.  This instead reports that the project has already been
harmed on _this_ system by such a filesystem after the fact.

So from the end-user's point of view, "will cause problems" may be a
message that came a bit too late.  "have collided and only one from
the same colliding group is in the working tree; others failed to be
checked out" is probably closer to the truth.

Re: [PATCH 1/1] pull --rebase=: allow single-letter abbreviations for the type

[Ævar Arnfjörð Bjarmason]

On Sat, Aug 04 2018, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin 
>
> Git for Windows' original 4aa8b8c8283 (Teach 'git pull' to handle
> --rebase=interactive, 2011-10-21) had support for the very convenient
> abbreviation
>
>   git pull --rebase=i
>
> which was later lost when it was ported to the builtin `git pull`, and
> it was not introduced before the patch eventually made it into Git as
> f5eb87b98dd (pull: allow interactive rebase with --rebase=interactive,
> 2016-01-13).
>
> However, it is *really* a useful short hand for the occasional rebasing
> pull on branches that do not usually want to be rebased.
>
> So let's reintroduce this convenience, at long last.

I agree with the shorthand, but I really think this should be
documented. The amount of people who'll discover this by reading the
code is much smaller than those who might discover it via the docs.

> Signed-off-by: Johannes Schindelin 
> ---
>  builtin/pull.c  |  6 +++---
>  t/t5520-pull.sh | 12 
>  2 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 4e7893539..53bc5facf 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -48,11 +48,11 @@ static enum rebase_type parse_config_rebase(const char 
> *key, const char *value,
>   return REBASE_FALSE;
>   else if (v > 0)
>   return REBASE_TRUE;
> - else if (!strcmp(value, "preserve"))
> + else if (!strcmp(value, "preserve") || !strcmp(value, "p"))
>   return REBASE_PRESERVE;
> - else if (!strcmp(value, "merges"))
> + else if (!strcmp(value, "merges") || !strcmp(value, "m"))
>   return REBASE_MERGES;
> - else if (!strcmp(value, "interactive"))
> + else if (!strcmp(value, "interactive") || !strcmp(value, "i"))
>   return REBASE_INTERACTIVE;

Here 3 special cases are added...

>   if (fatal)
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 68aa5f034..5e501c8b0 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -475,10 +475,22 @@ test_expect_success 'pull.rebase=interactive' '
>   false
>   EOF
>   test_set_editor "$TRASH_DIRECTORY/fake-editor" &&
> + test_when_finished "test_might_fail git rebase --abort" &&
>   test_must_fail git pull --rebase=interactive . copy &&
>   test "I was here" = "$(cat fake.out)"
>  '
>
> +test_expect_success 'pull --rebase=i' '
> + write_script "$TRASH_DIRECTORY/fake-editor" <<-\EOF &&
> + echo I was here, too >fake.out &&
> + false
> + EOF
> + test_set_editor "$TRASH_DIRECTORY/fake-editor" &&
> + test_when_finished "test_might_fail git rebase --abort" &&
> + test_must_fail git pull --rebase=i . copy &&
> + test "I was here, too" = "$(cat fake.out)"
> +'
> +
>  test_expect_success 'pull.rebase=invalid fails' '
>   git reset --hard before-preserve-rebase &&
>   test_config pull.rebase invalid &&

...but this test is only for 1/3. I haven't run this, but it looks like
the tests will still pass if we remove --rebase=p and --rebase=m.

Re: [PATCH] worktree: add --quiet option

[Eric Sunshine]

(cc:+Karen Arutyunov[1]; perhaps also do so when you re-roll)

In addition to the good review comments by Martin and Duy...

On Tue, Aug 7, 2018 at 9:21 AM Elia Pinto  wrote:
> Add the '--quiet' option to git worktree add,
> as for the other git commands.

It might make sense to say instead that this is adding a --quiet
option _in general_, rather than doing so specifically for 'add'.
Then, go on to say that, at present, 'add' is the only command
affected by it since all other commands are currently silent by
default (except, of course, 'list').

Whether you like that idea or not, as Martin suggested, please do say
something in the commit message about why the other commands don't
need it.

> Signed-off-by: Elia Pinto 
> ---
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> @@ -115,7 +115,9 @@ Unlock a working tree, allowing it to be pruned, moved or 
> deleted.
>  OPTIONS
>  ---
> -
> +-q::
> +--quiet::
> +   With 'add', suppress feedback messages.
>  -f::
>  --force::

In addition to the blank-line issues Martin raised, please move this
hunk downward to be a neighbor of the --verbose option.

REFERENCES
[1]: 
https://public-inbox.org/git/02659385-334f-2b77-c9a8-ffea8e461...@codesynthesis.com/

Re: [PATCH] travis-ci: include the trash directories of failed tests in the trace log

[SZEDER Gábor]

On Tue, Aug 7, 2018 at 5:12 PM Lars Schneider  wrote:
>
> > On Aug 1, 2018, at 12:56 AM, SZEDER Gábor  wrote:
> >
> > The trash directory of a failed test might contain invaluable
> > information about the cause of the failure, but we have no access to
> > the trash directories of Travis CI build jobs.  The only feedback we
> > get from there is the build job's trace log, so...
>
> 100% agree that keeping the trash directory is valuable.
>
> > Modify 'ci/print-test-failures.sh' to create a tar.gz archive of the
> > trash directory of each failed test, encode that archive with base64,
> > and print the resulting block of ASCII text, so it gets embedded in
> > the trace log.  Furthermore, run tests with '--immediate' to
> > faithfully preserve the failed state.
>
> I feel that might be complicated to work with.

It works quite conveniently in practice, that was one of the points of
the next paragraph.

> How about this:
> We create an publicly available Git repo (e.g. github.com/git/git-test-trash`)

I'm not sure I understand.  A publicly _writable_ repository?  That
sounds like asking for trouble.

> and then we push the trash directory of every failed test run to
> this repo. Then we link the commit in the logs to help people find
> them.
>
> Wouldn't that make it easier to access the trash dir?

With this patch, if there are any trash directories embedded in the
log, then the log will show a command near the very end that you can
copy-paste into a terminal, and bam! in less than 3 seconds you have
all those trash directories downloaded and unpacked.

I'm not sure how it could be made any easier than that.

> > Extracting the trash directories from the trace log turned out to be a
> > bit of a hassle, partly because of the size of these logs (usually
> > resulting in several hundreds or even thousands of lines of
> > base64-encoded text), and partly because these logs have CRLF, CRCRLF
> > and occasionally even CRCRCRLF line endings, which cause 'base64 -d'
> > from coreutils to complain about "invalid input".  For convenience add
> > a small script 'ci/util/extract-trash-dirs.sh', which will extract and
> > unpack all base64-encoded trash directories embedded in the log fed to
> > its standard input, and include an example command to be copy-pasted
> > into a terminal to do it all at the end of the failure report.
> >
> > A few of our tests create sizeable trash directories, so limit the
> > size of each included base64-encoded block, let's say, to 1MB.  And
> > just in case something fundamental gets broken and a lot of tests fail
> > at once, don't include trash directories when the combined size of the
> > included base64-encoded blocks would exceed 1MB.
> >
> > Signed-off-by: SZEDER Gábor 
> > ---

[PATCH v2] clone: report duplicate entries on case-insensitive filesystems

[Nguyễn Thái Ngọc Duy]

Paths that only differ in case work fine in a case-sensitive
filesystems, but if those repos are cloned in a case-insensitive one,
you'll get problems. The first thing to notice is "git status" will
never be clean with no indication what exactly is "dirty".

This patch helps the situation a bit by pointing out the problem at
clone time. Even though this patch talks about case sensitivity, the
patch makes no assumption about folding rules by the filesystem. It
simply observes that if an entry has been already checked out at clone
time when we're about to write a new path, some folding rules are
behind this.

I do not make any suggestions to fix or workaround the problem because
there are many different options, especially when the problem comes
from folding rules other than case (e.g. UTF-8 normalization, Windows
special paths...)

In the previous iteration, inode has been suggested to find the
matching entry. But it is platform specific, and because we already
have a common function for matching stat, the function is used here
even if it's more expensive. Bonus point is we don't need some "#ifdef
platform" around this code.

The cost goes higher when we find duplicated entries at the bottom of
the index, but the number of these entries should be very small that
total extra cost should not be really noticeable.

This patch is tested with vim-colorschemes repository on a JFS partion
with case insensitive support on Linux. This repository has two files
darkBlue.vim and darkblue.vim.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 v2 has completely different approach so no point in sending
 interdiff.

 One nice thing about this is we don't need platform specific code for
 detecting the duplicate entries. I think ce_match_stat() works even
 on Windows. And it's now equally expensive on all platforms :D

 builtin/clone.c |  1 +
 cache.h |  2 ++
 entry.c | 44 
 unpack-trees.c  | 23 +++
 unpack-trees.h  |  1 +
 5 files changed, 71 insertions(+)

diff --git a/builtin/clone.c b/builtin/clone.c
index 9ebb5acf56..38d5609282 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -748,6 +748,7 @@ static int checkout(int submodule_progress)
memset(, 0, sizeof opts);
opts.update = 1;
opts.merge = 1;
+   opts.clone = 1;
opts.fn = oneway_merge;
opts.verbose_update = (option_verbosity >= 0);
opts.src_index = _index;
diff --git a/cache.h b/cache.h
index 8dc7134f00..cdf0984707 100644
--- a/cache.h
+++ b/cache.h
@@ -1515,9 +1515,11 @@ struct checkout {
const char *base_dir;
int base_dir_len;
struct delayed_checkout *delayed_checkout;
+   int *nr_duplicates;
unsigned force:1,
 quiet:1,
 not_new:1,
+clone:1,
 refresh_cache:1;
 };
 #define CHECKOUT_INIT { NULL, "" }
diff --git a/entry.c b/entry.c
index b5d1d3cf23..3917bfc874 100644
--- a/entry.c
+++ b/entry.c
@@ -399,6 +399,47 @@ static int check_path(const char *path, int len, struct 
stat *st, int skiplen)
return lstat(path, st);
 }
 
+static void mark_duplicate_entries(const struct checkout *state,
+  struct cache_entry *ce, struct stat *st)
+{
+   int i;
+   int *count = state->nr_duplicates;
+
+   if (!count)
+   BUG("state->nr_duplicates must not be NULL");
+
+   ce->ce_flags |= CE_MATCHED;
+   (*count)++;
+
+   if (!state->refresh_cache)
+   BUG("We need this to narrow down the set of updated entries");
+
+   for (i = 0; i < state->istate->cache_nr; i++) {
+   struct cache_entry *dup = state->istate->cache[i];
+
+   /*
+* This entry has not been checked out yet, otherwise
+* its stat info must have been updated. And since we
+* check out from top to bottom, the rest is guaranteed
+* not checked out. Stop now.
+*/
+   if (!ce_uptodate(dup))
+   break;
+
+   if (dup->ce_flags & CE_MATCHED)
+   continue;
+
+   if (ce_match_stat(dup, st,
+ CE_MATCH_IGNORE_VALID |
+ CE_MATCH_IGNORE_SKIP_WORKTREE))
+   continue;
+
+   dup->ce_flags |= CE_MATCHED;
+   (*count)++;
+   break;
+   }
+}
+
 /*
  * Write the contents from ce out to the working tree.
  *
@@ -455,6 +496,9 @@ int checkout_entry(struct cache_entry *ce,
return -1;
}
 
+   if (state->clone)
+   mark_duplicate_entries(state, ce, );
+
/*
 * We unlink the old file, to get the new one with the
 * right permissions (including umask, which is nasty
diff --git a/unpack-trees.c b/unpack-trees.c
index 

Re: [PATCHv3 1/5] t7406: simplify by using diff --name-only instead of diff --raw

[Junio C Hamano]

Elijah Newren  writes:

>> I think 0/5 should fix the real bug you are deliberately keeping in
>> this patch, from the point of view of organization.
>
> You mean 5/5?  And yeah, it was just a temporary thing for
> organizational purposes.

I meant "a thing that comes before all the other steps".

As far as I can tell, everything else this 5-patch series improves
would still have caught a new bug in the subsystem being tested
(i.e. "submodule update") even without these improvements.  Surely,
if we greak "git diff", "git diff --raw" that sits on the upstream
of a pipe would not have stopped these tests, but we have tests for
"diff" elsewhere and t7406 are not about catching the breakage of
"diff", so from that point of view, fixing them, although it is
necessary and important, is less important.

The "\| grep" thing was a real bug in that if we broke "submodule
update", it would not have helped to catch such a bug, as it wasn't
looking for 'submodule' string in the output.

That is why I felt it would have been a better organization to fix
"\| grep" in "a thing that comes before all the other steps" and
then fix the rest as lower priority clean up.

Re: [PATCHv3 1/5] t7406: simplify by using diff --name-only instead of diff --raw

[Elijah Newren]

On Tue, Aug 7, 2018 at 10:29 AM Junio C Hamano  wrote:
> Elijah Newren  writes:
>
> > We can get rid of some quoted tabs and make a few tests slightly easier
> > to read and edit by just asking for the names of the files modified,
> > since that's all these tests were interested in anyway.
>
> Technically the quoted tab was making sure that we do not mistake
> "subsubmodule" (if existed) as "submodule" we seek, so a faithful
> replacement would be to find "^submodule", and "^submodule$" would
> be an improvement.  But we do not have paths with confusing names in
> these tests, so we can leave it as-is, I guess.

I knew someone would find additional issues.  I'll add the anchors if
any other issues come up in review for the series.

> I think 0/5 should fix the real bug you are deliberately keeping in
> this patch, from the point of view of organization.

You mean 5/5?  And yeah, it was just a temporary thing for
organizational purposes.

Re: [PATCHv3 5/5] t7406: fix call that was failing for the wrong reason

[Junio C Hamano]

Elijah Newren  writes:

> A test making use of test_must_fail was failing like this:
>   fatal: ambiguous argument '|': unknown revision or path not in the working 
> tree.
> when the intent was to verify that a specific string was not found
> in the output of the git diff command, i.e. that grep returned
> non-zero.  Fix the test to do that.

Nice.  All other patches and the final shape of this script look
sensible.

Thanks.

>
> Signed-off-by: Elijah Newren 
> ---
>  t/t7406-submodule-update.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index 5b42bbe9fa..7dd1c86b02 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -603,7 +603,8 @@ test_expect_success 'submodule update - update=none in 
> .git/config but --checkou
>git diff --name-only >out &&
>grep submodule out &&
>git submodule update --checkout &&
> -  test_must_fail git diff --name-only \| grep submodule &&
> +  git diff --name-only >out &&
> +  ! grep submodule out &&
>(cd submodule &&
> ! compare_head
>) &&

Re: [PATCHv3 3/5] t7406: prefer test_* helper functions to test -[feds]

[Junio C Hamano]

Elijah Newren  writes:

> test -e, test -s, etc. do not provide nice error messages when we hit
> test failures, so use the test_* helper functions from
> test-lib-functions.sh.

Good explanation.

> Note: The use of 'test_path_is_file submodule/.git' may look odd, but
> it is a file which is populated with a
>gitdir: ../.git/modules/submodule
> directive.  If, in the future, handling of the submodule is changed and
> submodule/.git becomes a directory we can change this to
> test_path_is_dir (or perhaps write a test_path_exists helper function
> that doesn't care whether the path is a file or a directory).

Yup, path_exists would be a good direction going forward.  If we
already have "missing" and use it in this rewrite, it may make sense
to introduce "exists" and use it at the same time here.

>
> Signed-off-by: Elijah Newren 
> ---
>  t/t7406-submodule-update.sh | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index c6b7b59350..ab67e373c5 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -174,7 +174,7 @@ test_expect_success 'submodule update does not fetch 
> already present commits' '
> git submodule update > ../actual 2> ../actual.err
>   ) &&
>   test_i18ncmp expected actual &&
> - ! test -s actual.err
> + test_must_be_empty actual.err
>  '
>  
>  test_expect_success 'submodule update should fail due to local changes' '
> @@ -619,8 +619,8 @@ test_expect_success 'submodule update --init skips 
> submodule with update=none' '
>   git clone super cloned &&
>   (cd cloned &&
>git submodule update --init &&
> -  test -e submodule/.git &&
> -  test_must_fail test -e none/.git
> +  test_path_is_file submodule/.git &&
> +  test_path_is_missing none/.git
>   )
>  '

Re: [GSoC][PATCH v5 02/20] rebase -i: rewrite append_todo_help() in C

[Phillip Wood]

Hi Christian
On 07/08/18 17:28, Christian Couder wrote:
> On Tue, Aug 7, 2018 at 6:15 PM, Phillip Wood  
> wrote:
>> On 07/08/18 16:25, Christian Couder wrote:
>>>
>>> I agree about checking the return value from fputs(), but it seems to
>>> me that we don't usually check the value of fclose().
>>
>> A quick grep shows you're right, there are only a handful of places where
>> the return value of fclose() is checked (there aren't many checks for the
>> return value of close() either), I'm don't think that is safe though given
>> that write errors may only show up when the file gets flushed by closing it.
> 
> I vaguely remember we tried to check those return values but it didn't
> work well sometimes.
> 

I think there's no point in checking the return value after a successful
read, or when writing and the file is being closed (and then possibly
removed) in some clean-up in an error path, but if the code cares about
ensuring the data is written then I think it should be checking the
return value, close_tempfile_gently() does for example.

In the sequencer code there are some examples where it probably should
be checking the return value, and places such as rewrite_file() and
write_message() (which uses the lockfile api) where the return code is
checked (there are a couple of recently added callers of write_message()
in the code to recreate octopus merges that don't check its return value
but all the other callers do).

Best Wishes

Phillip

Re: [PATCHv3 1/5] t7406: simplify by using diff --name-only instead of diff --raw

[Junio C Hamano]

Elijah Newren  writes:

> We can get rid of some quoted tabs and make a few tests slightly easier
> to read and edit by just asking for the names of the files modified,
> since that's all these tests were interested in anyway.

Technically the quoted tab was making sure that we do not mistake
"subsubmodule" (if existed) as "submodule" we seek, so a faithful
replacement would be to find "^submodule", and "^submodule$" would
be an improvement.  But we do not have paths with confusing names in
these tests, so we can leave it as-is, I guess.

I think 0/5 should fix the real bug you are deliberately keeping in
this patch, from the point of view of organization.


> Signed-off-by: Elijah Newren 
> ---
>  t/t7406-submodule-update.sh | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index f604ef7a72..e2405c96b5 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -579,9 +579,9 @@ test_expect_success 'submodule update - update=none in 
> .git/config' '
> git checkout master &&
> compare_head
>) &&
> -  git diff --raw | grep "submodule" &&
> +  git diff --name-only | grep submodule &&
>git submodule update &&
> -  git diff --raw | grep "submodule" &&
> +  git diff --name-only | grep submodule &&
>(cd submodule &&
> compare_head
>) &&
> @@ -597,9 +597,9 @@ test_expect_success 'submodule update - update=none in 
> .git/config but --checkou
> git checkout master &&
> compare_head
>) &&
> -  git diff --raw | grep "submodule" &&
> +  git diff --name-only | grep submodule &&
>git submodule update --checkout &&
> -  test_must_fail git diff --raw \| grep "submodule" &&
> +  test_must_fail git diff --name-only \| grep submodule &&
>(cd submodule &&
> test_must_fail compare_head
>) &&

Re: [PATCH v3 2/2] sequencer: fix quoting in write_author_script

[Junio C Hamano]

Phillip Wood  writes:

> Yes I think the earlier approach with the more robust detection you
> suggested is probably a good compromise. Junio does that sound good to
> you?

Surely, and thanks.

[PATCHv3 2/5] t7406: avoid having git commands upstream of a pipe

[Elijah Newren]

When a git command is on the left side of a pipe, the pipe will swallow
its exit status, preventing us from detecting failures in said commands.
Restructure the tests to put the output in a temporary file to avoid
this problem.

Signed-off-by: Elijah Newren 
---
 t/t7406-submodule-update.sh | 26 +-
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index e2405c96b5..c6b7b59350 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -481,7 +481,8 @@ test_expect_success 'recursive submodule update - command 
in .git/config catches
 
 test_expect_success 'submodule init does not copy command into .git/config' '
(cd super &&
-H=$(git ls-files -s submodule | cut -d" " -f2) &&
+git ls-files -s submodule >out &&
+H=$(cut -d" " -f2 out) &&
 mkdir submodule1 &&
 git update-index --add --cacheinfo 16 $H submodule1 &&
 git config -f .gitmodules submodule.submodule1.path submodule1 &&
@@ -579,9 +580,11 @@ test_expect_success 'submodule update - update=none in 
.git/config' '
  git checkout master &&
  compare_head
 ) &&
-git diff --name-only | grep submodule &&
+git diff --name-only >out &&
+grep submodule out &&
 git submodule update &&
-git diff --name-only | grep submodule &&
+git diff --name-only >out &&
+grep submodule out &&
 (cd submodule &&
  compare_head
 ) &&
@@ -597,7 +600,8 @@ test_expect_success 'submodule update - update=none in 
.git/config but --checkou
  git checkout master &&
  compare_head
 ) &&
-git diff --name-only | grep submodule &&
+git diff --name-only >out &&
+grep submodule out &&
 git submodule update --checkout &&
 test_must_fail git diff --name-only \| grep submodule &&
 (cd submodule &&
@@ -885,7 +889,8 @@ test_expect_success 'submodule update properly revives a 
moved submodule' '
 H=$(git rev-parse --short HEAD) &&
 git commit -am "pre move" &&
 H2=$(git rev-parse --short HEAD) &&
-git status | sed "s/$H/XXX/" >expect &&
+git status >out &&
+sed "s/$H/XXX/" out >expect &&
 H=$(cd submodule2 && git rev-parse HEAD) &&
 git rm --cached submodule2 &&
 rm -rf submodule2 &&
@@ -894,7 +899,8 @@ test_expect_success 'submodule update properly revives a 
moved submodule' '
 git config -f .gitmodules submodule.submodule2.path "moved/sub module" 
&&
 git commit -am "post move" &&
 git submodule update &&
-git status | sed "s/$H2/XXX/" >actual &&
+git status > out &&
+sed "s/$H2/XXX/" out >actual &&
 test_cmp expect actual
)
 '
@@ -912,7 +918,7 @@ test_expect_success SYMLINKS 'submodule update can handle 
symbolic links in pwd'
 
 test_expect_success 'submodule update clone shallow submodule' '
test_when_finished "rm -rf super3" &&
-   first=$(git -C cloned submodule status submodule |cut -c2-41) &&
+   first=$(git -C cloned rev-parse HEAD:submodule) &&
second=$(git -C submodule rev-parse HEAD) &&
commit_count=$(git -C submodule rev-list --count $first^..$second) &&
git clone cloned super3 &&
@@ -922,7 +928,8 @@ test_expect_success 'submodule update clone shallow 
submodule' '
sed -e "s#url = ../#url = file://$pwd/#" <.gitmodules 
>.gitmodules.tmp &&
mv -f .gitmodules.tmp .gitmodules &&
git submodule update --init --depth=$commit_count &&
-   test 1 = $(git -C submodule log --oneline | wc -l)
+   git -C submodule log --oneline >out &&
+   test_line_count = 1 out
)
 '
 
@@ -938,7 +945,8 @@ test_expect_success 'submodule update clone shallow 
submodule outside of depth'
test_i18ngrep "Direct fetching of that commit failed." actual &&
git -C ../submodule config uploadpack.allowReachableSHA1InWant 
true &&
git submodule update --init --depth=1 >actual &&
-   test 1 = $(git -C submodule log --oneline | wc -l)
+   git -C submodule log --oneline >out &&
+   test_line_count = 1 out
)
 '
 
-- 
2.18.0.550.geb414df874.dirty

[PATCHv3 5/5] t7406: fix call that was failing for the wrong reason

[Elijah Newren]

A test making use of test_must_fail was failing like this:
  fatal: ambiguous argument '|': unknown revision or path not in the working 
tree.
when the intent was to verify that a specific string was not found
in the output of the git diff command, i.e. that grep returned
non-zero.  Fix the test to do that.

Signed-off-by: Elijah Newren 
---
 t/t7406-submodule-update.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 5b42bbe9fa..7dd1c86b02 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -603,7 +603,8 @@ test_expect_success 'submodule update - update=none in 
.git/config but --checkou
 git diff --name-only >out &&
 grep submodule out &&
 git submodule update --checkout &&
-test_must_fail git diff --name-only \| grep submodule &&
+git diff --name-only >out &&
+! grep submodule out &&
 (cd submodule &&
  ! compare_head
 ) &&
-- 
2.18.0.550.geb414df874.dirty

[PATCHv3 0/5] Simple fixes to t7406

[Elijah Newren]

This series started as a simple single-line fix, but folks keep
noticing other problems with t7406 while reading my patches.  So, changes
noted below and I have a challenge at the end.

Changes since v2:
  - Two new patches inserted into this series (3/5 and 4/5):
- Prefer test_* helper functions to 'test -[feds]' (see patch 3; this
  is my attempt to remove easy answers from the challenge below)
- Fix other places in the testfile using test_must_fail for non-git
  commands (see patch 4; suggested by SZEDER)
  - Avoid losing the check of git's exit status in a command substituion,
improve potential error message for a test while at it (part of patch 2;
suggested by SZEDER)
  - Update the commit message for the final commit to match the updated
code (Pointed out by Martin)

Since folks like to notice other problems with t7406 while reading my
patches, here's a challenge:

  Find something *else* wrong with t7406 that neither I nor any of the
  reviewers so far have caught that could be fixed.

- You get bonus points if that thing is in the context region for
  one of my five patches.
- Extra bonus points if the thing needing fixing was on a line I
  changed.
- You win outright if it's something big enough that I give up and
  request to just have my series merged as-is and punt your
  suggested fixes down the road to someone else.

  (Note: If I flubbed something in v3, pointing it out doesn't count
   as finding "something else", but do please still point it out.)

:-)


Elijah Newren (5):
  t7406: simplify by using diff --name-only instead of diff --raw
  t7406: avoid having git commands upstream of a pipe
  t7406: prefer test_* helper functions to test -[feds]
  t7406: avoid using test_must_fail for commands other than git
  t7406: fix call that was failing for the wrong reason

 t/t7406-submodule-update.sh | 37 +++--
 1 file changed, 23 insertions(+), 14 deletions(-)

-- 
2.18.0.550.geb414df874.dirty

[PATCHv3 4/5] t7406: avoid using test_must_fail for commands other than git

[Elijah Newren]

Signed-off-by: Elijah Newren 
---
 t/t7406-submodule-update.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index ab67e373c5..5b42bbe9fa 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -605,7 +605,7 @@ test_expect_success 'submodule update - update=none in 
.git/config but --checkou
 git submodule update --checkout &&
 test_must_fail git diff --name-only \| grep submodule &&
 (cd submodule &&
- test_must_fail compare_head
+ ! compare_head
 ) &&
 git config --unset submodule.submodule.update
)
-- 
2.18.0.550.geb414df874.dirty

[PATCHv3 1/5] t7406: simplify by using diff --name-only instead of diff --raw

[Elijah Newren]

We can get rid of some quoted tabs and make a few tests slightly easier
to read and edit by just asking for the names of the files modified,
since that's all these tests were interested in anyway.

Signed-off-by: Elijah Newren 
---
 t/t7406-submodule-update.sh | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index f604ef7a72..e2405c96b5 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -579,9 +579,9 @@ test_expect_success 'submodule update - update=none in 
.git/config' '
  git checkout master &&
  compare_head
 ) &&
-git diff --raw | grep "submodule" &&
+git diff --name-only | grep submodule &&
 git submodule update &&
-git diff --raw | grep "submodule" &&
+git diff --name-only | grep submodule &&
 (cd submodule &&
  compare_head
 ) &&
@@ -597,9 +597,9 @@ test_expect_success 'submodule update - update=none in 
.git/config but --checkou
  git checkout master &&
  compare_head
 ) &&
-git diff --raw | grep "submodule" &&
+git diff --name-only | grep submodule &&
 git submodule update --checkout &&
-test_must_fail git diff --raw \| grep "submodule" &&
+test_must_fail git diff --name-only \| grep submodule &&
 (cd submodule &&
  test_must_fail compare_head
 ) &&
-- 
2.18.0.550.geb414df874.dirty

[PATCHv3 3/5] t7406: prefer test_* helper functions to test -[feds]

[Elijah Newren]

test -e, test -s, etc. do not provide nice error messages when we hit
test failures, so use the test_* helper functions from
test-lib-functions.sh.

Note: The use of 'test_path_is_file submodule/.git' may look odd, but
it is a file which is populated with a
   gitdir: ../.git/modules/submodule
directive.  If, in the future, handling of the submodule is changed and
submodule/.git becomes a directory we can change this to
test_path_is_dir (or perhaps write a test_path_exists helper function
that doesn't care whether the path is a file or a directory).

Signed-off-by: Elijah Newren 
---
 t/t7406-submodule-update.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index c6b7b59350..ab67e373c5 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -174,7 +174,7 @@ test_expect_success 'submodule update does not fetch 
already present commits' '
  git submodule update > ../actual 2> ../actual.err
) &&
test_i18ncmp expected actual &&
-   ! test -s actual.err
+   test_must_be_empty actual.err
 '
 
 test_expect_success 'submodule update should fail due to local changes' '
@@ -619,8 +619,8 @@ test_expect_success 'submodule update --init skips 
submodule with update=none' '
git clone super cloned &&
(cd cloned &&
 git submodule update --init &&
-test -e submodule/.git &&
-test_must_fail test -e none/.git
+test_path_is_file submodule/.git &&
+test_path_is_missing none/.git
)
 '
 
-- 
2.18.0.550.geb414df874.dirty

Re: [GSoC][PATCH v5 02/20] rebase -i: rewrite append_todo_help() in C

[Christian Couder]

On Tue, Aug 7, 2018 at 6:15 PM, Phillip Wood  wrote:
> On 07/08/18 16:25, Christian Couder wrote:
>>
>> I agree about checking the return value from fputs(), but it seems to
>> me that we don't usually check the value of fclose().
>
> A quick grep shows you're right, there are only a handful of places where
> the return value of fclose() is checked (there aren't many checks for the
> return value of close() either), I'm don't think that is safe though given
> that write errors may only show up when the file gets flushed by closing it.

I vaguely remember we tried to check those return values but it didn't
work well sometimes.

Re: [GSoC][PATCH v5 02/20] rebase -i: rewrite append_todo_help() in C

[Phillip Wood]

Hi Christian
On 07/08/18 16:25, Christian Couder wrote:

Hi Phillip,

On Tue, Aug 7, 2018 at 3:57 PM, Phillip Wood  wrote:


On 31/07/18 18:59, Alban Gruin wrote:


+
+ ret = fputs(buf.buf, todo);


It is not worth changing the patch just for this but strbuf_write()
might be clearer (you use it in a later patch)


+ if (ret < 0)
+ error_errno(_("could not append help text to '%s'"), 
rebase_path_todo());
+
+ fclose(todo);


You should definitely check the return value and return an error if
appropriate as fputs() might not actually write any data until you try
and close the file.


I agree about checking the return value from fputs(), but it seems to
me that we don't usually check the value of fclose().


A quick grep shows you're right, there are only a handful of places 
where the return value of fclose() is checked (there aren't many checks 
for the return value of close() either), I'm don't think that is safe 
though given that write errors may only show up when the file gets 
flushed by closing it.


Best Wishes

Phillip


Thanks,
Christian.

Re: [PATCH] Documentation/diff-options: explain different diff algorithms

[Junio C Hamano]

Jonathan Nieder  writes:

> Both don't seem quite right, since they have an extra space before the
> period.  The git-diff(1) seems especially not quite right --- does it
> intend to say something like "See the DIFF ALGORITHMS section for more
> discussion"?

Good suggestion and doing so would nicely allow side-stepping the
space before the dot issue, i.e. "See the DIFF ALGO section (in
git-diff(1)) for more discussion".   I like it.

> [...]
>> --- a/Documentation/git-diff.txt
>> +++ b/Documentation/git-diff.txt
>> @@ -119,6 +119,40 @@ include::diff-options.txt[]
>>  
>>  include::diff-format.txt[]
>>  
>> +DIFF ALGORITHMS
>> +---
>
> Please add some introductory words about what the headings refer to.
>
>> +`Myers`
>> +
>> +A diff as produced by the basic greedy algorithm described in
>> +link:http://www.xmailserver.org/diff2.pdf[An O(ND) Difference Algorithm and 
>> its Variations].
>> +...
>> +
>> +`Patience`
>> +
>> +This algorithm by Bram Cohen matches the longest common subsequence
>> +...
> [...]
>> +`Histogram`
>> +
>> +This algorithm finds the longest common substring and recursively
>> +diffs the content before and after the longest common substring.

As a first-time reader, I felt that it is a bit uneven that there is
no attribution for only this item, unlike the description for Myers
and Patience.

Re: [PATCH] worktree: add --quiet option

[Elia Pinto]

2018-08-07 16:37 GMT+02:00 Martin Ågren :
> Hi Elia
>
> On 7 August 2018 at 15:21, Elia Pinto  wrote:
>> Add the '--quiet' option to git worktree add,
>> as for the other git commands.
>>
>> Signed-off-by: Elia Pinto 
>> ---
>>  Documentation/git-worktree.txt |  4 +++-
>>  builtin/worktree.c | 11 +--
>>  2 files changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
>> index 9c26be40f..508cde55c 100644
>> --- a/Documentation/git-worktree.txt
>> +++ b/Documentation/git-worktree.txt
>> @@ -115,7 +115,9 @@ Unlock a working tree, allowing it to be pruned, moved 
>> or deleted.
>>
>>  OPTIONS
>>  ---
>> -
>
> Grepping through Documentation/, it is clear that we sometimes have a
> blank line here, sometimes not. I'm not sure what to make of that.
>
>> +-q::
>> +--quiet::
>> +   With 'add', suppress feedback messages.
>>  -f::
>
> But I do think that for consistency, we'd prefer a blank line before `-f::`.
>
> Both the commit message and this documentation makes me wonder if this
> focuses on "add" because it's the only subcommand where `--quiet` makes
> sense, conceptually, or because this is where you happen to need it
> personally, or due to some other $reason. Could you say something more
> about this?
>
> I'm not a worktree power-user, so please forgive my ignorance...
>
>> @@ -315,6 +316,9 @@ static int add_worktree(const char *path, const char 
>> *refname,
>> cp.argv = NULL;
>> argv_array_clear();
>> argv_array_pushl(, "reset", "--hard", NULL);
>> +   if (opts->quiet)
>> +   argv_array_push(, "--quiet");
>> +   printf("%s\n","soo qia");
>
> This last line looks like debug cruft.
>
>> @@ -437,6 +441,7 @@ static int add(int ac, const char **av, const char 
>> *prefix)
>> OPT_BOOL(0, "detach", , N_("detach HEAD at named 
>> commit")),
>> OPT_BOOL(0, "checkout", , N_("populate the new 
>> working tree")),
>> OPT_BOOL(0, "lock", _locked, N_("keep the new 
>> working tree locked")),
>> +   OPT__QUIET(, N_("suppress progress reporting")),
>
> This matches other users. Good.
>
> I did some simple testing and this appears to be quite quiet, modulo
> the "soo qia" that I already mentioned. Could you add a test to
> demonstrate the quietness and to keep it from regressing? Something like
> `git worktree add ../foo >out && test_must_be_empty out" in e.g.,
> t2025-worktree-add.sh might do the trick (capture stderr as well?).
>
> Hope this helps
> Martin


Thank you all. sorry for the trash in the patch I will resend it.

Re: What's cooking in git.git (Aug 2018, #01; Thu, 2)

[Jakub Narębski]

On Tue, 7 Aug 2018 at 16:36, Junio C Hamano  wrote:
> Jakub Narebski  writes:
>> Junio C Hamano  writes:
>>
>>> * ds/commit-graph-with-grafts (2018-07-19) 8 commits
>>>   (merged to 'next' on 2018-08-02 at 0ee624e329)
>>> ...
>>>  Will merge to 'master'.
>>
>> Derrick wrote that he will be sending v2 of this patch series in a few
>> weeks, among others to make it use commit-graph feature if replace
>> objects are present but not used (as some git hosting services do, see
>> core.useReplaceRefs below).
>
> Ah, thanks for stopping me (albeit a bit too late-ish, but reverting
> that merge is easy enough).  Do you have a handy reference to stop
> me from making the same mistake on this topic later?

https://public-inbox.org/git/a3640919-95cf-cca4-d552-4715a031d...@gmail.com/

DS> Since this series now has two dependencies (including Stefan's ref-store
DS> fix that I had included in my v1), I'll let those topics settle
DS> before I send a new v2.
DS>
DS> If there are more comments about how I'm handling these cases, then
DS> please jump in and tell me. I'll revisit this topic in a few weeks.

>> Also, the test for interaction of commit-graph with the grafts file
>> feature does not actually test grafts, but replace objects.

https://public-inbox.org/git/86bmap7l7a@gmail.com/

>>> * jk/core-use-replace-refs (2018-07-18) 3 commits
>>>   (merged to 'next' on 2018-08-02 at 90fb6b1056)
>>>  + add core.usereplacerefs config option
>>>  + check_replace_refs: rename to read_replace_refs
>>>  + check_replace_refs: fix outdated comment
>>>
>>>  A new configuration variable core.usereplacerefs has been added,
>>>  primarily to help server installations that want to ignore the
>>>  replace mechanism altogether.
>>>
>>>  Will merge to 'master'.
[...]
-- 
Jakub Narębski

Re: [PATCH] worktree: add --quiet option

[Duy Nguyen]

On Tue, Aug 7, 2018 at 3:27 PM Elia Pinto  wrote:
>
> Add the '--quiet' option to git worktree add,
> as for the other git commands.
>
> Signed-off-by: Elia Pinto 
> ---
>  Documentation/git-worktree.txt |  4 +++-
>  builtin/worktree.c | 11 +--
>  2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> index 9c26be40f..508cde55c 100644
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -115,7 +115,9 @@ Unlock a working tree, allowing it to be pruned, moved or 
> deleted.
>
>  OPTIONS
>  ---
> -
> +-q::
> +--quiet::
> +   With 'add', suppress feedback messages.

Should we update the synopsis as well?

> @@ -315,6 +316,9 @@ static int add_worktree(const char *path, const char 
> *refname,

Before here we run either update-ref or symbolic-ref. update-ref does
not have --quiet so it's fine, no need to add another option there
(until it shows something when used with "worktree add --quiet") but
symbolic-ref seems to support -q. Should we pass -q to it?

> cp.argv = NULL;
> argv_array_clear();
> argv_array_pushl(, "reset", "--hard", NULL);
> +   if (opts->quiet)
> +   argv_array_push(, "--quiet");
> +   printf("%s\n","soo qia");
> cp.env = child_env.argv;
> ret = run_command();
> if (ret)
> @@ -437,6 +441,7 @@ static int add(int ac, const char **av, const char 
> *prefix)
> OPT_BOOL(0, "detach", , N_("detach HEAD at named 
> commit")),
> OPT_BOOL(0, "checkout", , N_("populate the new 
> working tree")),
> OPT_BOOL(0, "lock", _locked, N_("keep the new 
> working tree locked")),
> +   OPT__QUIET(, N_("suppress progress reporting")),

git grep OPT__QUIET shows that we have plenty different messages to
describe --quiet. But yeah "support progress reporting" seems close
enough in this context.

> OPT_PASSTHRU(0, "track", _track, NULL,
>  N_("set up tracking mode (see git-branch(1))"),
>  PARSE_OPT_NOARG | PARSE_OPT_OPTARG),

The rest looks good.
-- 
Duy

Re: [GSoC][PATCH v5 02/20] rebase -i: rewrite append_todo_help() in C

[Christian Couder]

Hi Phillip,

On Tue, Aug 7, 2018 at 3:57 PM, Phillip Wood  wrote:
>
> On 31/07/18 18:59, Alban Gruin wrote:
>>
>> +
>> + ret = fputs(buf.buf, todo);
>
> It is not worth changing the patch just for this but strbuf_write()
> might be clearer (you use it in a later patch)
>
>> + if (ret < 0)
>> + error_errno(_("could not append help text to '%s'"), 
>> rebase_path_todo());
>> +
>> + fclose(todo);
>
> You should definitely check the return value and return an error if
> appropriate as fputs() might not actually write any data until you try
> and close the file.

I agree about checking the return value from fputs(), but it seems to
me that we don't usually check the value of fclose().

Thanks,
Christian.

Re: What's cooking in git.git (Aug 2018, #02; Mon, 6)

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> On Mon, Aug 06 2018, Junio C Hamano wrote:
>
>> * ab/newhash-is-sha256 (2018-08-06) 2 commits
>>  - doc hash-function-transition: pick SHA-256 as NewHash
>>  - doc hash-function-transition: note the lack of a changelog
>>
>>  Documentation update.
>>
>>  Will Merge to 'next'.
>
> The tip of gitster/ab/newhash-is-sha256 has a unicode-corrupted
> signed-off-by line from me. See
> https://github.com/git/git/commit/f855a19d45 not sure how that got
> there...

Hmph, it came from this message (most headers omitted)

To: =?iso-8859-1?Q?=C6var_Arnfj=F6r=F0?= Bjarmason 
Message-ID: <20180804085247.ge55...@aiede.svl.corp.google.com>
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

Subject: doc hash-function-transition: pick SHA-256 as NewHash

...

Signed-off-by: Ævar Arnfjörð Bjarmason 

and the body seems to be correct iso-8859-1.  "od -cx" tells me that
the file stores 0xf0 for that D looking thing, for example.  Could it
be mailinfo that screws up, I wonder.  A quick check with "git mailinfo"
does not give me anything suspicous---the info contents emitted to
its standard output is correctly converted to UTF-8.  Puzzled...

I read from public-inbox/git over nntp, if that matters.

Re: What's cooking in git.git (Aug 2018, #02; Mon, 6)

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> On Mon, Aug 06 2018, Junio C Hamano wrote:
>
>> * ab/newhash-is-sha256 (2018-08-06) 2 commits
>>  - doc hash-function-transition: pick SHA-256 as NewHash
>>  - doc hash-function-transition: note the lack of a changelog
>>
>>  Documentation update.
>>
>>  Will Merge to 'next'.
>
> The tip of gitster/ab/newhash-is-sha256 has a unicode-corrupted
> signed-off-by line from me. See
> https://github.com/git/git/commit/f855a19d45 not sure how that got
> there...

Thanks for noticing and reporting.  Will correct.

Re: [PATCH] worktree: add --quiet option

[Martin Ågren]

Hi Elia

On 7 August 2018 at 15:21, Elia Pinto  wrote:
> Add the '--quiet' option to git worktree add,
> as for the other git commands.
>
> Signed-off-by: Elia Pinto 
> ---
>  Documentation/git-worktree.txt |  4 +++-
>  builtin/worktree.c | 11 +--
>  2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> index 9c26be40f..508cde55c 100644
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -115,7 +115,9 @@ Unlock a working tree, allowing it to be pruned, moved or 
> deleted.
>
>  OPTIONS
>  ---
> -

Grepping through Documentation/, it is clear that we sometimes have a
blank line here, sometimes not. I'm not sure what to make of that.

> +-q::
> +--quiet::
> +   With 'add', suppress feedback messages.
>  -f::

But I do think that for consistency, we'd prefer a blank line before `-f::`.

Both the commit message and this documentation makes me wonder if this
focuses on "add" because it's the only subcommand where `--quiet` makes
sense, conceptually, or because this is where you happen to need it
personally, or due to some other $reason. Could you say something more
about this?

I'm not a worktree power-user, so please forgive my ignorance...

> @@ -315,6 +316,9 @@ static int add_worktree(const char *path, const char 
> *refname,
> cp.argv = NULL;
> argv_array_clear();
> argv_array_pushl(, "reset", "--hard", NULL);
> +   if (opts->quiet)
> +   argv_array_push(, "--quiet");
> +   printf("%s\n","soo qia");

This last line looks like debug cruft.

> @@ -437,6 +441,7 @@ static int add(int ac, const char **av, const char 
> *prefix)
> OPT_BOOL(0, "detach", , N_("detach HEAD at named 
> commit")),
> OPT_BOOL(0, "checkout", , N_("populate the new 
> working tree")),
> OPT_BOOL(0, "lock", _locked, N_("keep the new 
> working tree locked")),
> +   OPT__QUIET(, N_("suppress progress reporting")),

This matches other users. Good.

I did some simple testing and this appears to be quite quiet, modulo
the "soo qia" that I already mentioned. Could you add a test to
demonstrate the quietness and to keep it from regressing? Something like
`git worktree add ../foo >out && test_must_be_empty out" in e.g.,
t2025-worktree-add.sh might do the trick (capture stderr as well?).

Hope this helps
Martin

Re: What's cooking in git.git (Aug 2018, #01; Thu, 2)

[Junio C Hamano]

Jakub Narebski  writes:

> Junio C Hamano  writes:
>
>> * ds/commit-graph-with-grafts (2018-07-19) 8 commits
>>   (merged to 'next' on 2018-08-02 at 0ee624e329)
>> ...
>>  Will merge to 'master'.
>
> Derrick wrote that he will be sending v2 of this patch series in a few
> weeks, among others to make it use commit-graph feature if replace
> objects are present but not used (as some git hosting services do, see
> core.useReplaceRefs below).

Ah, thanks for stopping me (albeit a bit too late-ish, but reverting
that merge is easy enough).  Do you have a handy reference to stop
me from making the same mistake on this topic later?

>
> Also, the test for interaction of commit-graph with the grafts file
> feature does not actually test grafts, but replace objects.
>
>> * jk/core-use-replace-refs (2018-07-18) 3 commits
>>   (merged to 'next' on 2018-08-02 at 90fb6b1056)
>>  + add core.usereplacerefs config option
>>  + check_replace_refs: rename to read_replace_refs
>>  + check_replace_refs: fix outdated comment
>>
>>  A new configuration variable core.usereplacerefs has been added,
>>  primarily to help server installations that want to ignore the
>>  replace mechanism altogether.
>>
>>  Will merge to 'master'.
>
> Nice to have features used in the wild merged into core git.

Yes, indeed.

Re: [GSoC][PATCH v5 04/20] rebase -i: rewrite the edit-todo functionality in C

[Phillip Wood]

On 31/07/18 18:59, Alban Gruin wrote:
> This rewrites the edit-todo functionality from shell to C.
> 
> To achieve that, a new command mode, `edit-todo`, is added, and the
> `write-edit-todo` flag is removed, as the shell script does not need to
> write the edit todo help message to the todo list anymore.
> 
> The shell version is then stripped in favour of a call to the helper.
> 
> Signed-off-by: Alban Gruin 
> ---
> No changes since v4.
> 
>  builtin/rebase--helper.c   | 13 -
>  git-rebase--interactive.sh | 11 +--
>  rebase-interactive.c   | 31 +++
>  rebase-interactive.h   |  1 +
>  4 files changed, 41 insertions(+), 15 deletions(-)
> 
> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
> index 05e73e71d4..731a64971d 100644
> --- a/builtin/rebase--helper.c
> +++ b/builtin/rebase--helper.c
> @@ -13,12 +13,12 @@ static const char * const builtin_rebase_helper_usage[] = 
> {
>  int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>  {
>   struct replay_opts opts = REPLAY_OPTS_INIT;
> - unsigned flags = 0, keep_empty = 0, rebase_merges = 0, write_edit_todo 
> = 0;
> + unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
>   int abbreviate_commands = 0, rebase_cousins = -1;
>   enum {
>   CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
>   CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
> - ADD_EXEC, APPEND_TODO_HELP
> + ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO
>   } command = 0;
>   struct option options[] = {
>   OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
> @@ -28,8 +28,6 @@ int cmd_rebase__helper(int argc, const char **argv, const 
> char *prefix)
>   OPT_BOOL(0, "rebase-merges", _merges, N_("rebase merge 
> commits")),
>   OPT_BOOL(0, "rebase-cousins", _cousins,
>N_("keep original branch points of cousins")),
> - OPT_BOOL(0, "write-edit-todo", _edit_todo,
> -  N_("append the edit-todo message to the todo-list")),
>   OPT_CMDMODE(0, "continue", , N_("continue rebase"),
>   CONTINUE),
>   OPT_CMDMODE(0, "abort", , N_("abort rebase"),
> @@ -50,6 +48,9 @@ int cmd_rebase__helper(int argc, const char **argv, const 
> char *prefix)
>   N_("insert exec commands in todo list"), ADD_EXEC),
>   OPT_CMDMODE(0, "append-todo-help", ,
>   N_("insert the help in the todo list"), 
> APPEND_TODO_HELP),
> + OPT_CMDMODE(0, "edit-todo", ,
> + N_("edit the todo list during an interactive 
> rebase"),
> + EDIT_TODO),
>   OPT_END()
>   };
>  
> @@ -90,6 +91,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
> char *prefix)
>   if (command == ADD_EXEC && argc == 2)
>   return !!sequencer_add_exec_commands(argv[1]);
>   if (command == APPEND_TODO_HELP && argc == 1)
> - return !!append_todo_help(write_edit_todo, keep_empty);
> + return !!append_todo_help(0, keep_empty);
> + if (command == EDIT_TODO && argc == 1)
> + return !!edit_todo_list(flags);
>   usage_with_options(builtin_rebase_helper_usage, options);
>  }
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 94c23a7af2..2defe607f4 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -108,16 +108,7 @@ initiate_action () {
>--continue
>   ;;
>   edit-todo)
> - git stripspace --strip-comments <"$todo" >"$todo".new
> - mv -f "$todo".new "$todo"
> - collapse_todo_ids
> - git rebase--helper --append-todo-help --write-edit-todo
> -
> - git_sequence_editor "$todo" ||
> - die "$(gettext "Could not execute editor")"
> - expand_todo_ids
> -
> - exit
> + exec git rebase--helper --edit-todo
>   ;;
>   show-current-patch)
>   exec git show REBASE_HEAD --
> diff --git a/rebase-interactive.c b/rebase-interactive.c
> index d7996bc8d9..403ecbf3c9 100644
> --- a/rebase-interactive.c
> +++ b/rebase-interactive.c
> @@ -66,3 +66,34 @@ int append_todo_help(unsigned edit_todo, unsigned 
> keep_empty)
>  
>   return ret;
>  }
> +
> +int edit_todo_list(unsigned flags)
> +{
> + struct strbuf buf = STRBUF_INIT;
> + const char *todo_file = rebase_path_todo();
> + FILE *todo;
> +
> + if (strbuf_read_file(, todo_file, 0) < 0)
> + return error_errno(_("could not read '%s'."), todo_file);
> +
> + strbuf_stripspace(, 1);
> + todo = fopen_or_warn(todo_file, "w");

This truncates the existing file, if there are any errors writing the
new one then the user has lost the old one. write_message() in

Re: [GSoC][PATCH v5 02/20] rebase -i: rewrite append_todo_help() in C

[Phillip Wood]

Hi Alban

On 31/07/18 18:59, Alban Gruin wrote:
> This rewrites append_todo_help() from shell to C. It also incorporates
> some parts of initiate_action() and complete_action() that also write
> help texts to the todo file.
> 
> This also introduces the source file rebase-interactive.c. This file
> will contain functions necessary for interactive rebase that are too
> specific for the sequencer, and is part of libgit.a.
> 
> Two flags are added to rebase--helper.c: one to call
> append_todo_help() (`--append-todo-help`), and another one to tell
> append_todo_help() to write the help text suited for the edit-todo
> mode (`--write-edit-todo`).
> 
> Finally, append_todo_help() is removed from git-rebase--interactive.sh
> to use `rebase--helper --append-todo-help` instead.
> 
> Signed-off-by: Alban Gruin 
> ---
> No changes since v4.
> 
>  Makefile   |  1 +
>  builtin/rebase--helper.c   | 11 --
>  git-rebase--interactive.sh | 52 ++---
>  rebase-interactive.c   | 68 ++
>  rebase-interactive.h   |  6 
>  5 files changed, 86 insertions(+), 52 deletions(-)
>  create mode 100644 rebase-interactive.c
>  create mode 100644 rebase-interactive.h
> 
> diff --git a/Makefile b/Makefile
> index 08e5c54549..909a687857 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -922,6 +922,7 @@ LIB_OBJS += protocol.o
>  LIB_OBJS += quote.o
>  LIB_OBJS += reachable.o
>  LIB_OBJS += read-cache.o
> +LIB_OBJS += rebase-interactive.o
>  LIB_OBJS += reflog-walk.o
>  LIB_OBJS += refs.o
>  LIB_OBJS += refs/files-backend.o
> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
> index f7c2a5fdc8..05e73e71d4 100644
> --- a/builtin/rebase--helper.c
> +++ b/builtin/rebase--helper.c
> @@ -3,6 +3,7 @@
>  #include "config.h"
>  #include "parse-options.h"
>  #include "sequencer.h"
> +#include "rebase-interactive.h"
>  
>  static const char * const builtin_rebase_helper_usage[] = {
>   N_("git rebase--helper []"),
> @@ -12,12 +13,12 @@ static const char * const builtin_rebase_helper_usage[] = 
> {
>  int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>  {
>   struct replay_opts opts = REPLAY_OPTS_INIT;
> - unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
> + unsigned flags = 0, keep_empty = 0, rebase_merges = 0, write_edit_todo 
> = 0;
>   int abbreviate_commands = 0, rebase_cousins = -1;
>   enum {
>   CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
>   CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
> - ADD_EXEC
> + ADD_EXEC, APPEND_TODO_HELP
>   } command = 0;
>   struct option options[] = {
>   OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
> @@ -27,6 +28,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
> char *prefix)
>   OPT_BOOL(0, "rebase-merges", _merges, N_("rebase merge 
> commits")),
>   OPT_BOOL(0, "rebase-cousins", _cousins,
>N_("keep original branch points of cousins")),
> + OPT_BOOL(0, "write-edit-todo", _edit_todo,
> +  N_("append the edit-todo message to the todo-list")),
>   OPT_CMDMODE(0, "continue", , N_("continue rebase"),
>   CONTINUE),
>   OPT_CMDMODE(0, "abort", , N_("abort rebase"),
> @@ -45,6 +48,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
> char *prefix)
>   N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
>   OPT_CMDMODE(0, "add-exec-commands", ,
>   N_("insert exec commands in todo list"), ADD_EXEC),
> + OPT_CMDMODE(0, "append-todo-help", ,
> + N_("insert the help in the todo list"), 
> APPEND_TODO_HELP),
>   OPT_END()
>   };
>  
> @@ -84,5 +89,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
> char *prefix)
>   return !!rearrange_squash();
>   if (command == ADD_EXEC && argc == 2)
>   return !!sequencer_add_exec_commands(argv[1]);
> + if (command == APPEND_TODO_HELP && argc == 1)
> + return !!append_todo_help(write_edit_todo, keep_empty);
>   usage_with_options(builtin_rebase_helper_usage, options);
>  }
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 299ded2137..94c23a7af2 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -39,38 +39,6 @@ comment_for_reflog () {
>   esac
>  }
>  
> -append_todo_help () {
> - gettext "
> -Commands:
> -p, pick  = use commit
> -r, reword  = use commit, but edit the commit message
> -e, edit  = use commit, but stop for amending
> -s, squash  = use commit, but meld into previous commit
> -f, fixup  = like \"squash\", but discard this commit's log message
> -x, exec  = run command (the rest of the line) using shell
> -d, drop  = remove commit
> -l, 

Re: [PATCH v4 2/2] sequencer: fix quoting in write_author_script

[Phillip Wood]

Hi Eric

On 07/08/18 11:23, Eric Sunshine wrote:
> On Tue, Aug 7, 2018 at 5:35 AM Phillip Wood  wrote:
>>  - Reverted the implementation to v2 with more robust detection of the
>>missing "'" on the last line of the author script based on a
>>suggestion by Eric. This means that this series needs to progress
>>closely with Eric's series of fixes or the fallback code will never
>>be called.
> 
> Thanks for working on this. I haven't read the patch closely yet, but
> one thing caught my attention as I ran my eye over it...
> 
>> +static int quoting_is_broken(const char *s, size_t n)
>> +{
>> +   /* Skip any empty lines in case the file was hand edited */
>> +   while (n > 0 && s[--n] == '\n')
>> +   ; /* empty */
>> +   if (n > 0 && s[n] != '\'')
>> +   return 1;
> 
> To be "technically correct", I think the condition in the 'if'
> statement should be ">=". It should never happen in practice that the
> entire content of the file is a single character followed by zero or
> more newlines, but using the proper condition ">=" would save future
> readers of this code a "huh?" moment.
> 

I'm not sure it is that simple. If the script consists solely of a
single quote then we should return 1, if it is a single character that
is not "'" then it should return 0. Currently it returns 0 in both those
cases so is technically broken when the script is "'". If it used ">="
instead then I think it would return 0 when it should return 1 and vice
versa. As you say this shouldn't happen in practice.

Best Wishes

Phillip

[RFC PATCH] line-log: clarify [a,b) notation for ranges

[Andrei Rybak]

line-log.[ch] use left-closed, right-open interval logic. Change comment
and debug output to square brackets+parentheses notation to help
developers avoid off-by-one errors.
---

Original idea for this change in recent thread about line-log changes:

  https://public-inbox.org/git/9776862d-18b2-43ec-cfeb-829418d4d...@gmail.com/

line-log.c also uses ASCII graphics |---| in some comments, like:

/*
 * a: |---
 * b: --|
 */

But I'm not sure if replacing them with

/*
 * a: [---
 * b: --)
 */

will be helpful.  Another possibility is to update comment for
range_set_append_unsafe to read

  /* tack on a _new_ range [a,b) _at the end_ */
  void range_set_append_unsafe(struct range_set *rs, long a, long b)

 line-log.c | 4 ++--
 line-log.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/line-log.c b/line-log.c
index fa9cfd5bd..60f3174ac 100644
--- a/line-log.c
+++ b/line-log.c
@@ -353,7 +353,7 @@ static void dump_range_set(struct range_set *rs, const char 
*desc)
int i;
printf("range set %s (%d items):\n", desc, rs->nr);
for (i = 0; i < rs->nr; i++)
-   printf("\t[%ld,%ld]\n", rs->ranges[i].start, rs->ranges[i].end);
+   printf("\t[%ld,%ld)\n", rs->ranges[i].start, rs->ranges[i].end);
 }
 
 static void dump_line_log_data(struct line_log_data *r)
@@ -373,7 +373,7 @@ static void dump_diff_ranges(struct diff_ranges *diff, 
const char *desc)
printf("diff ranges %s (%d items):\n", desc, diff->parent.nr);
printf("\tparent\ttarget\n");
for (i = 0; i < diff->parent.nr; i++) {
-   printf("\t[%ld,%ld]\t[%ld,%ld]\n",
+   printf("\t[%ld,%ld)\t[%ld,%ld)\n",
   diff->parent.ranges[i].start,
   diff->parent.ranges[i].end,
   diff->target.ranges[i].start,
diff --git a/line-log.h b/line-log.h
index e2a5ee7c6..488c86409 100644
--- a/line-log.h
+++ b/line-log.h
@@ -6,7 +6,7 @@
 struct rev_info;
 struct commit;
 
-/* A range [start,end].  Lines are numbered starting at 0, and the
+/* A range [start,end).  Lines are numbered starting at 0, and the
  * ranges include start but exclude end. */
 struct range {
long start, end;
-- 
2.18.0

Re: [PATCH 3/3] t7406: make a test_must_fail call fail for the right reason

[Elijah Newren]

On Tue, Aug 7, 2018 at 2:07 AM, SZEDER Gábor  wrote:
>> A test making use of test_must_fail was failing like this:
>>   fatal: ambiguous argument '|': unknown revision or path not in the working 
>> tree.
>> when the intent was to verify that a specific string was not found
>> in the output of the git diff command, i.e. that grep returned
>> non-zero.  Fix the test to do that.
>>
>> Signed-off-by: Elijah Newren 
>> ---
>>  t/t7406-submodule-update.sh | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
>> index c8971abd07..f65049ec73 100755
>> --- a/t/t7406-submodule-update.sh
>> +++ b/t/t7406-submodule-update.sh
>> @@ -603,7 +603,8 @@ test_expect_success 'submodule update - update=none in 
>> .git/config but --checkou
>>git diff --name-only >out &&
>>grep submodule out &&
>>git submodule update --checkout &&
>> -  test_must_fail git diff --name-only \| grep submodule &&
>> +  git diff --name-only >out &&
>> +  ! grep submodule out &&
>>(cd submodule &&
>> test_must_fail compare_head
>
> May I suggest a while-at-it cleanup? :)

I think while-at-it-cleanups have become the primary purpose of this
series.  :-)

> Here 'test_must_fail' is used in front of a shell function, which
> should be written as '! compare_head', and indeed in all other places
> in this test script it's written that way.

Good catch; I'll fix it up.  In fact, there's also one other
test_must_fail in front of a non-git command inside t7406, so I'll fix
that up while at it too.

Re: [PATCH 0/2] Simple fixes to t7406

[Elijah Newren]

On Tue, Aug 7, 2018 at 12:50 AM, Martin Ågren  wrote:
> On 6 August 2018 at 17:25, Elijah Newren  wrote:
>> Changes since v1:
>>   - Simplify multiple tests using diff --name-only instead of diff --raw;
>> these tests are only interested in which file was modified anyway.
>> (Suggested by Junio)
>>   - Avoid putting git commands upstream of a pipe, since we don't run under
>> set -o pipefail (Suggested by Eric)
>>   - Avoid using test_must_fail for system binaries (Pointed out by
>> both Eric and Junio)
>>
>> Elijah Newren (2):
>
> I'm not sure what's up with the count of 2 vs 3.

I started writing a cover letter, then realized I missed something in
the original review.  Fixed it up by adding another patch, but decided
to just manually "fix up" my cover letter to match -- and missed
something.  Oops.

>>   t7406: simplify by using diff --name-only instead of diff --raw
>>   t7406: avoid having git commands upstream of a pipe
>>   t7406: make a test_must_fail call fail for the right reason
>
> The subject of the final patch doesn't quite match its content anymore.
> The problematic test_must_fail is dropped, so it can no longer fail.
> Maybe something like "t7406: fix call that was failing for the wrong
> reason", or just s/test_must_fail// in what you have.

Good point; I'll fix it up.

[PATCH] worktree: add --quiet option

[Elia Pinto]

Add the '--quiet' option to git worktree add,
as for the other git commands.

Signed-off-by: Elia Pinto 
---
 Documentation/git-worktree.txt |  4 +++-
 builtin/worktree.c | 11 +--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 9c26be40f..508cde55c 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -115,7 +115,9 @@ Unlock a working tree, allowing it to be pruned, moved or 
deleted.
 
 OPTIONS
 ---
-
+-q::
+--quiet::
+   With 'add', suppress feedback messages.
 -f::
 --force::
By default, `add` refuses to create a new working tree when
diff --git a/builtin/worktree.c b/builtin/worktree.c
index a763dbdcc..c47feb4a4 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -27,6 +27,7 @@ static const char * const worktree_usage[] = {
 struct add_opts {
int force;
int detach;
+   int quiet;
int checkout;
int keep_locked;
 };
@@ -315,6 +316,9 @@ static int add_worktree(const char *path, const char 
*refname,
cp.argv = NULL;
argv_array_clear();
argv_array_pushl(, "reset", "--hard", NULL);
+   if (opts->quiet)
+   argv_array_push(, "--quiet");
+   printf("%s\n","soo qia");
cp.env = child_env.argv;
ret = run_command();
if (ret)
@@ -437,6 +441,7 @@ static int add(int ac, const char **av, const char *prefix)
OPT_BOOL(0, "detach", , N_("detach HEAD at named 
commit")),
OPT_BOOL(0, "checkout", , N_("populate the new 
working tree")),
OPT_BOOL(0, "lock", _locked, N_("keep the new working 
tree locked")),
+   OPT__QUIET(, N_("suppress progress reporting")),
OPT_PASSTHRU(0, "track", _track, NULL,
 N_("set up tracking mode (see git-branch(1))"),
 PARSE_OPT_NOARG | PARSE_OPT_OPTARG),
@@ -491,8 +496,8 @@ static int add(int ac, const char **av, const char *prefix)
}
}
}
-
-   print_preparing_worktree_line(opts.detach, branch, new_branch, 
!!new_branch_force);
+   if (!opts.quiet)
+   print_preparing_worktree_line(opts.detach, branch, new_branch, 
!!new_branch_force);
 
if (new_branch) {
struct child_process cp = CHILD_PROCESS_INIT;
@@ -500,6 +505,8 @@ static int add(int ac, const char **av, const char *prefix)
argv_array_push(, "branch");
if (new_branch_force)
argv_array_push(, "--force");
+   if (opts.quiet)
+   argv_array_push(, "--quiet");
argv_array_push(, new_branch);
argv_array_push(, branch);
if (opt_track)
-- 
2.18.0.721.gc1f18ed

[PATCH v7 1/1] sideband: highlight keywords in remote sideband output

[Han-Wen Nienhuys]

The colorization is controlled with the config setting "color.remote".

Supported keywords are "error", "warning", "hint" and "success". They
are highlighted if they appear at the start of the line, which is
common in error messages, eg.

   ERROR: commit is missing Change-Id

The Git push process itself prints lots of non-actionable messages
(eg. bandwidth statistics, object counters for different phases of the
process). This obscures actionable error messages that servers may
send back. Highlighting keywords in the sideband draws more attention
to those messages.

The background for this change is that Gerrit does server-side
processing to create or update code reviews, and actionable error
messages (eg. missing Change-Id) must be communicated back to the user
during the push. User research has shown that new users have trouble
seeing these messages.

The highlighting is done on the client rather than server side, so
servers don't have to grow capabilities to understand terminal escape
codes and terminal state. It also consistent with the current state
where Git is control of the local display (eg. prefixing messages with
"remote: ").

The highlighting can be configured using color.remote.
configuration settings. Since the keys are matched case insensitively,
we match the keywords case insensitively too.

Finally, this solution is backwards compatible: many servers already
prefix their messages with "error", and they will benefit from this
change without requiring a server update. By contrast, a server-side
solution would likely require plumbing the TERM variable through the
git protocol, so it would require changes to both server and client.

Helped-by: Duy Nguyen 
Signed-off-by: Han-Wen Nienhuys 
---
 Documentation/config.txt|  12 +++
 help.c  |   1 +
 help.h  |   1 +
 sideband.c  | 125 ++--
 t/t5409-colorize-remote-messages.sh |  87 +++
 5 files changed, 217 insertions(+), 9 deletions(-)
 create mode 100755 t/t5409-colorize-remote-messages.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 63365dcf3d..33bc1a3def 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1263,6 +1263,18 @@ color.push::
 color.push.error::
Use customized color for push errors.
 
+color.remote::
+   If set, keywords at the start of the line are highlighted. The
+   keywords are "error", "warning", "hint" and "success", and are
+   matched case-insensitively. Maybe set to `always`, `false` (or
+   `never`) or `auto` (or `true`). If unset, then the value of
+   `color.ui` is used (`auto` by default).
+
+color.remote.::
+   Use customized color for each remote keyword. `` may be
+   `hint`, `warning`, `success` or `error` which match the
+   corresponding keyword.
+
 color.showBranch::
A boolean to enable/disable color in the output of
linkgit:git-show-branch[1]. May be set to `always`,
diff --git a/help.c b/help.c
index 3ebf0568db..b6cafcfc0a 100644
--- a/help.c
+++ b/help.c
@@ -425,6 +425,7 @@ void list_config_help(int for_human)
{ "color.diff", "", list_config_color_diff_slots },
{ "color.grep", "", list_config_color_grep_slots },
{ "color.interactive", "", 
list_config_color_interactive_slots },
+   { "color.remote", "", list_config_color_sideband_slots },
{ "color.status", "", list_config_color_status_slots },
{ "fsck", "", list_config_fsck_msg_ids },
{ "receive.fsck", "", list_config_fsck_msg_ids },
diff --git a/help.h b/help.h
index f8b15323a6..9eab6a3f89 100644
--- a/help.h
+++ b/help.h
@@ -83,6 +83,7 @@ void list_config_color_diff_slots(struct string_list *list, 
const char *prefix);
 void list_config_color_grep_slots(struct string_list *list, const char 
*prefix);
 void list_config_color_interactive_slots(struct string_list *list, const char 
*prefix);
 void list_config_color_status_slots(struct string_list *list, const char 
*prefix);
+void list_config_color_sideband_slots(struct string_list *list, const char 
*prefix);
 void list_config_fsck_msg_ids(struct string_list *list, const char *prefix);
 
 #endif /* HELP_H */
diff --git a/sideband.c b/sideband.c
index 325bf0e974..1c6bb0e25b 100644
--- a/sideband.c
+++ b/sideband.c
@@ -1,6 +1,109 @@
 #include "cache.h"
+#include "color.h"
+#include "config.h"
 #include "pkt-line.h"
 #include "sideband.h"
+#include "help.h"
+
+struct keyword_entry {
+   /*
+* We use keyword as config key so it should be a single alphanumeric 
word.
+*/
+   const char *keyword;
+   char color[COLOR_MAXLEN];
+};
+
+static struct keyword_entry keywords[] = {
+   { "hint",   GIT_COLOR_YELLOW },
+   { "warning",GIT_COLOR_BOLD_YELLOW },
+   { "success",GIT_COLOR_BOLD_GREEN },
+   { "error",  

[PATCH v7 0/1] sideband: highlight keywords in remote sideband output

[Han-Wen Nienhuys]

Fix nits; remove debug printf.

Han-Wen Nienhuys (1):
  sideband: highlight keywords in remote sideband output

 Documentation/config.txt|  12 +++
 help.c  |   1 +
 help.h  |   1 +
 sideband.c  | 125 ++--
 t/t5409-colorize-remote-messages.sh |  87 +++
 5 files changed, 217 insertions(+), 9 deletions(-)
 create mode 100755 t/t5409-colorize-remote-messages.sh

--
2.18.0.597.ga71716f1ad-goog

Re: [PATCH 1/1] pull --rebase=: allow single-letter abbreviations for the type

[Johannes Schindelin]

Hi Junio,

On Mon, 6 Aug 2018, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" 
> writes:
> 
> > From: Johannes Schindelin 
> >
> > Git for Windows' original 4aa8b8c8283 (Teach 'git pull' to handle
> > --rebase=interactive, 2011-10-21) had support for the very convenient
> > abbreviation
> >
> > git pull --rebase=i
> >
> > which was later lost when it was ported to the builtin `git pull`, and
> > it was not introduced before the patch eventually made it into Git as
> > f5eb87b98dd (pull: allow interactive rebase with --rebase=interactive,
> > 2016-01-13).
> >
> > However, it is *really* a useful short hand for the occasional rebasing
> > pull on branches that do not usually want to be rebased.
> >
> > So let's reintroduce this convenience, at long last.
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> 
> Makes sense, whether this patch is adding back what in the past
> existed only in the Windows port but lost, and whether the lossage
> was long time ago or in just a few releases go, or it is adding a
> complete new feature for that matter.  This looks like a good
> short-hand.
> 
> Will queue.

Thanks!
Dscho

Re: [PATCH v3] t4150: fix broken test for am --scissors

[Andrei Rybak]

On 2018-08-06 22:14, Junio C Hamano wrote:
> Andrei Rybak  writes:
>>
>> Only changes since v2 are more clear tag names.
> 
> ... and updated log message, which I think makes it worthwhile to
> replace the previous one plus the squash/fixup with this version.

My bad, totally forgot that I had been tweaking it after I sent out v2.

Re: [PATCH v2] t4150: fix broken test for am --scissors

[Paul Tan]

On Tue, Aug 7, 2018 at 1:42 AM, Andrei Rybak  wrote:
> On 2018-08-06 10:58, Paul Tan wrote:
>>> +   git commit -F msg-without-scissors-line &&
>>> +   git tag scissors-used &&
>>
>> Nit: I'm not quite sure about naming the tag "scissors-used" though,
>> since this commit was not created from the output of "git am
>> --scissors". Maybe it should be named `commit-without-scissors-line`
>> or something?
>>
>>> +   git commit -F msg-with-scissors-line &&
>>> +   git tag scissors-not-used &&
>>
>> Nit: Likewise, perhaps this tag could be named `commit-with-scissors-line`?
>
> How about "expected-for-scissors" and "expected-for-no-scissors"?

Yep that's fine.

Thanks,
Paul

Re: What's cooking in git.git (Aug 2018, #01; Thu, 2)

[Jakub Narebski]

Junio C Hamano  writes:

> * ds/commit-graph-with-grafts (2018-07-19) 8 commits
>   (merged to 'next' on 2018-08-02 at 0ee624e329)
>  + commit-graph: close_commit_graph before shallow walk
>  + commit-graph: not compatible with uninitialized repo
>  + commit-graph: not compatible with grafts
>  + commit-graph: not compatible with replace objects
>  + test-repository: properly init repo
>  + commit-graph: update design document
>  + refs.c: upgrade for_each_replace_ref to be a each_repo_ref_fn callback
>  + refs.c: migrate internal ref iteration to pass thru repository argument
>
>  The recently introduced commit-graph auxiliary data is incompatible
>  with mechanisms such as replace & grafts that "breaks" immutable
>  nature of the object reference relationship.  Disable optimizations
>  based on its use (and updating existing commit-graph) when these
>  incompatible features are in use in the repository.
>
>  Will merge to 'master'.

Derrick wrote that he will be sending v2 of this patch series in a few
weeks, among others to make it use commit-graph feature if replace
objects are present but not used (as some git hosting services do, see
core.useReplaceRefs below).

Also, the test for interaction of commit-graph with the grafts file
feature does not actually test grafts, but replace objects.

> * jk/core-use-replace-refs (2018-07-18) 3 commits
>   (merged to 'next' on 2018-08-02 at 90fb6b1056)
>  + add core.usereplacerefs config option
>  + check_replace_refs: rename to read_replace_refs
>  + check_replace_refs: fix outdated comment
>
>  A new configuration variable core.usereplacerefs has been added,
>  primarily to help server installations that want to ignore the
>  replace mechanism altogether.
>
>  Will merge to 'master'.

Nice to have features used in the wild merged into core git.

-- 
Jakub Narębski

Re: [PATCH v4 2/2] sequencer: fix quoting in write_author_script

[Eric Sunshine]

On Tue, Aug 7, 2018 at 5:35 AM Phillip Wood  wrote:
>  - Reverted the implementation to v2 with more robust detection of the
>missing "'" on the last line of the author script based on a
>suggestion by Eric. This means that this series needs to progress
>closely with Eric's series of fixes or the fallback code will never
>be called.

Thanks for working on this. I haven't read the patch closely yet, but
one thing caught my attention as I ran my eye over it...

> +static int quoting_is_broken(const char *s, size_t n)
> +{
> +   /* Skip any empty lines in case the file was hand edited */
> +   while (n > 0 && s[--n] == '\n')
> +   ; /* empty */
> +   if (n > 0 && s[n] != '\'')
> +   return 1;

To be "technically correct", I think the condition in the 'if'
statement should be ">=". It should never happen in practice that the
entire content of the file is a single character followed by zero or
more newlines, but using the proper condition ">=" would save future
readers of this code a "huh?" moment.

[PATCH v4 0/2] fix author-script quoting

[Phillip Wood]

From: Phillip Wood 

I've updated these based on Eric's suggestions, hopefully they're good
to go now. Thanks Eric for you help.

Phillip Wood (2):
  sequencer: handle errors from read_author_ident()
  sequencer: fix quoting in write_author_script

 sequencer.c   | 47 ---
 t/t3404-rebase-interactive.sh | 18 +++---
 2 files changed, 53 insertions(+), 12 deletions(-)

-- 
2.18.0

[PATCH v4 1/2] sequencer: handle errors from read_author_ident()

[Phillip Wood]

From: Phillip Wood 

Check for a NULL return value from read_author_ident() that indicates
an error. Previously the NULL author was passed to commit_tree() which
would then fallback to using the default author when creating the new
commit. This changed the date and potentially the author of the commit
which corrupted the author data compared to its expected value.

Helped-by: Eric Sunshine 
Signed-off-by: Phillip Wood 
---

Notes:
changes since v3:

 - Implemented the simpler scheme suggested by Eric

 sequencer.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 944dea6997..c4e4418559 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -795,11 +795,18 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
 
if ((flags & CREATE_ROOT_COMMIT) && !(flags & AMEND_MSG)) {
struct strbuf msg = STRBUF_INIT, script = STRBUF_INIT;
-   const char *author = is_rebase_i(opts) ?
-   read_author_ident() : NULL;
+   const char *author = NULL;
struct object_id root_commit, *cache_tree_oid;
int res = 0;
 
+   if (is_rebase_i(opts)) {
+   author = read_author_ident();
+   if (!author) {
+   strbuf_release();
+   return -1;
+   }
+   }
+
if (!defmsg)
BUG("root commit without message");
 
-- 
2.18.0

[PATCH v4 2/2] sequencer: fix quoting in write_author_script

[Phillip Wood]

From: Phillip Wood 

Single quotes should be escaped as \' not \\'. The bad quoting breaks
the interactive version of 'rebase --root' (which is used when there
is no '--onto' even if the user does not specify --interactive) for
authors that contain "'" as sq_dequote() called by read_author_ident()
errors out on the bad quoting.

For other interactive rebases this only affects external scripts that
read the author script and users whose git is upgraded from the shell
version of rebase -i while rebase was stopped when the author contains
"'". This is because the parsing in read_env_script() expected the
broken quoting.

This patch includes code to handle the broken quoting when
git has been upgraded while rebase was stopped. It does this by
detecting the missing "'" at the end of the GIT_AUTHOR_DATE line to see
if it should dequote \\' as "'". Note this is only implemented for
normal picks, not for creating a new root commit (rebase will stop with
an error complaining out bad quoting in that case).

The fallback code has been manually tested by reverting both the quoting
fixes in write_author_script() and the previous fix for the missing "'"
at the end of the GIT_AUTHOR_DATE line and running
t3404-rebase-interactive.sh.

Ideally rebase and am would share the same code for reading and
writing the author script, but this commit just fixes the immediate
bug.

Helped-by: Eric Sunshine 
Helped-by: Johannes Schindelin 
Signed-off-by: Phillip Wood 
---

Notes:
changes since v3:

 - Reverted the implementation to v2 with more robust detection of the
   missing "'" on the last line of the author script based on a
   suggestion by Eric. This means that this series needs to progress
   closely with Eric's series of fixes or the fallback code will never
   be called.

- Tweaked the last test

 sequencer.c   | 36 ---
 t/t3404-rebase-interactive.sh | 18 +++---
 2 files changed, 44 insertions(+), 10 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index c4e4418559..ba11fe5bca 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -636,42 +636,64 @@ static int write_author_script(const char *message)
else if (*message != '\'')
strbuf_addch(, *(message++));
else
-   strbuf_addf(, "'%c'", *(message++));
+   strbuf_addf(, "'\\%c'", *(message++));
strbuf_addstr(, "'\nGIT_AUTHOR_EMAIL='");
while (*message && *message != '\n' && *message != '\r')
if (skip_prefix(message, "> ", ))
break;
else if (*message != '\'')
strbuf_addch(, *(message++));
else
-   strbuf_addf(, "'%c'", *(message++));
+   strbuf_addf(, "'\\%c'", *(message++));
strbuf_addstr(, "'\nGIT_AUTHOR_DATE='@");
while (*message && *message != '\n' && *message != '\r')
if (*message != '\'')
strbuf_addch(, *(message++));
else
-   strbuf_addf(, "'%c'", *(message++));
+   strbuf_addf(, "'\\%c'", *(message++));
strbuf_addch(, '\'');
res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);
strbuf_release();
return res;
 }
 
+
+/*
+ * write_author_script() used to fail to terminate the last line with a "'" and
+ * also escaped "'" incorrectly as "'''" rather than "'\\''". We check for
+ * the terminating "'" on the last line to see how "'" has been escaped in case
+ * git was upgraded while rebase was stopped.
+ */
+static int quoting_is_broken(const char *s, size_t n)
+{
+   /* Skip any empty lines in case the file was hand edited */
+   while (n > 0 && s[--n] == '\n')
+   ; /* empty */
+   if (n > 0 && s[n] != '\'')
+   return 1;
+
+   return 0;
+}
+
 /*
  * Read a list of environment variable assignments (such as the author-script
  * file) into an environment block. Returns -1 on error, 0 otherwise.
  */
 static int read_env_script(struct argv_array *env)
 {
struct strbuf script = STRBUF_INIT;
-   int i, count = 0;
-   char *p, *p2;
+   int i, count = 0, sq_bug;
+   const char *p2;
+   char *p;
 
if (strbuf_read_file(, rebase_path_author_script(), 256) <= 0)
return -1;
-
+   /* write_author_script() used to quote incorrectly */
+   sq_bug = quoting_is_broken(script.buf, script.len);
for (p = script.buf; *p; p++)
-   if (skip_prefix(p, "'''", (const char **)))
+   if (sq_bug && skip_prefix(p, "'''", ))
+   strbuf_splice(, p - script.buf, p2 - p, "'", 1);
+   else if (skip_prefix(p, "'\\''", ))
strbuf_splice(, p - script.buf, p2 - p, "'", 1);
else if (*p == '\'')
 

Re: [RFC PATCH v2 06/12] submodule--helper: add a '--stage' option to the 'config' sub command

[Antonio Ospite]

On Mon, 06 Aug 2018 10:38:20 -0700
Junio C Hamano  wrote:

> Antonio Ospite  writes:
> 
> >> I also do not see a reason why we want to stop referring to
> >> .gitmodules explicitly by name.  We do not hide the fact that
> >> in-tree .gitignore and .gitattributes files are used to hold the
> >> metainformation about the project tree, saying that it is an
> >> implementation detail.  Is there a good reason why .gitmodules
> >> should be different from these other two?
> >
> > Not sure about that, but one difference I can see
> > between .gitignore/.gitattributes and .gitmodules is that I got the
> > impression that editing the latter by hand is strongly discouraged, if
> > that is indeed the case a layer of indirection can make sense IMHO to
> > make the actual file path less relevant.
> 
> I do not think we discourage hand editing of .gitmodules more than
> others, say .gitignore; and I do not see a sane reason to do so.
> 
> "If you commit broken .gitmodules and let another person clone it,
> submodules will not be checked out correctly" is *not* a sane
> reason, as exactly the same thing can be said for incorrect checkout
> of files with broken .gitattributes.
>

OK, I see, maybe I got the wrong impression because
while .gitignore/.gitattributes are expected to be hand edited, in my
limited usage of submodules I never had to edit .gitmodules manually
since git did it for me; I guess that does not necessarily mean it's
discouraged and it may even be necessary for more advanced usages.

> Quite honestly, I just want to get over with this minor detail that
> won't help any scripts (after all submodule--helper is not meant to
> be used by humans) and focus on other parts of the patch series.

Agreed, let's drop 06 and 07 then.

Thanks,
   Antonio

-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [PATCH 4/4] line-log: convert an assertion to a full BUG() call

[Eric Sunshine]

On Mon, Aug 6, 2018 at 9:15 AM Johannes Schindelin
 wrote:
> On Sun, 5 Aug 2018, Eric Sunshine wrote:
> > Although this appears to be a faithful translation of the assert() to
> > BUG(), as mentioned by Andrei in his review of 3/4, the existing
> > assert() seems to have an off-by-1 error, which means that the "> a"
> > here really ought to be ">= a".
>
> I think Andrei's assessment is wrong. The code could not test for that
> earlier, as it did allow ranges to become "abutting" in the process, by
> failing to merge them. So the invariant you talked about is more of an
> invariant for the initial state.

I'm having trouble interpreting your response.

My understanding is that range_set_append() is intended to be strict
by not allowing addition of a range which abuts an existing range
(although, of course, the assert() checks only the last range, so it's
not full-proof). Assuming that to be correct, then the assertion
contains a one-off-error (according to my reasoning).

I'm sensing from your reply that you seem to have a different idea
about range_set_append()'s intended use.

> My 3/4 would make that invariant heeded throughout the process.
>
> I am still keen on keeping the invariants straight *without* resorting to
> dirty tricks like calling sort_and_merge. Calling that function would just
> make it easier for bugs to hide in this code.

Indeed, avoiding the "dirty trick" would be ideal, although, I still
haven't wrapped my head around it enough to be able to say whether the
computed offset, when applied to the range in question, could cause
that range to abut or overlap an existing range.

(There are, of course, valid uses for range_set_append_unsafe() /
sort_and_merge(), such as allowing -L options to overlap and be
specified in any order. Batch-adding them to the range-set via
range_set_append_unsafe() and letting sort_and_merge() sort them all
out is plenty sensible.)

Re: [PATCH 3/3] t7406: make a test_must_fail call fail for the right reason

[SZEDER Gábor]

> A test making use of test_must_fail was failing like this:
>   fatal: ambiguous argument '|': unknown revision or path not in the working 
> tree.
> when the intent was to verify that a specific string was not found
> in the output of the git diff command, i.e. that grep returned
> non-zero.  Fix the test to do that.
> 
> Signed-off-by: Elijah Newren 
> ---
>  t/t7406-submodule-update.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index c8971abd07..f65049ec73 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -603,7 +603,8 @@ test_expect_success 'submodule update - update=none in 
> .git/config but --checkou
>git diff --name-only >out &&
>grep submodule out &&
>git submodule update --checkout &&
> -  test_must_fail git diff --name-only \| grep submodule &&
> +  git diff --name-only >out &&
> +  ! grep submodule out &&
>(cd submodule &&
> test_must_fail compare_head

May I suggest a while-at-it cleanup? :)
Here 'test_must_fail' is used in front of a shell function, which
should be written as '! compare_head', and indeed in all other places
in this test script it's written that way.

>) &&
> --
> 2.18.0.548.gd57a518419
> 
> 

[PATCH 4/5] chainlint: recognize multi-line quoted strings more robustly

[Eric Sunshine]

chainlint.sed recognizes multi-line quoted strings within subshells:

echo "abc
def" >out &&

so it can avoid incorrectly classifying lines internal to the string as
breaking the &&-chain. To identify the first line of a multi-line
string, it checks if the line contains a single quote. However, this is
fragile and can be easily fooled by a line containing multiple strings:

echo "xyz" "abc
def" >out &&

Make detection more robust by checking for an odd number of quotes
rather than only a single one.

(Escaped quotes are not handled, but support may be added later.)

The original multi-line string recognizer rather cavalierly threw away
all but the final quote, whereas the new one is careful to retain all
quotes, so the "expected" output of a couple existing chainlint tests is
updated to account for this new behavior.

Signed-off-by: Eric Sunshine 
---
 t/chainlint.sed   | 32 +--
 t/chainlint/here-doc-multi-line-string.expect |  2 +-
 t/chainlint/multi-line-string.expect  | 10 --
 t/chainlint/multi-line-string.test| 12 +++
 4 files changed, 43 insertions(+), 13 deletions(-)

diff --git a/t/chainlint.sed b/t/chainlint.sed
index 6c891bf383..338163c2f5 100644
--- a/t/chainlint.sed
+++ b/t/chainlint.sed
@@ -151,10 +151,10 @@ s/.*\n//
 :slurp
 # incomplete line "...\"
 /\\$/bincomplete
-# multi-line quoted string "...\n..."
-/^[^"]*"[^"]*$/bdqstring
-# multi-line quoted string '...\n...' (but not contraction in string "it's so")
-/^[^']*'[^']*$/{
+# multi-line quoted string "...\n..."?
+/"/bdqstring
+# multi-line quoted string '...\n...'? (but not contraction in string "it's")
+/'/{
/"[^'"]*'[^'"]*"/!bsqstring
 }
 :folded
@@ -250,20 +250,32 @@ N
 s/\\\n//
 bslurp
 
-# found multi-line double-quoted string "...\n..." -- slurp until end of string
+# check for multi-line double-quoted string "...\n..." -- fold to one line
 :dqstring
-s/"//g
+# remove all quote pairs
+s/"\([^"]*\)"/@!\1@!/g
+# done if no dangling quote
+/"/!bdqdone
+# otherwise, slurp next line and try again
 N
 s/\n//
-/"/!bdqstring
+bdqstring
+:dqdone
+s/@!/"/g
 bfolded
 
-# found multi-line single-quoted string '...\n...' -- slurp until end of string
+# check for multi-line single-quoted string '...\n...' -- fold to one line
 :sqstring
-s/'//g
+# remove all quote pairs
+s/'\([^']*\)'/@!\1@!/g
+# done if no dangling quote
+/'/!bsqdone
+# otherwise, slurp next line and try again
 N
 s/\n//
-/'/!bsqstring
+bsqstring
+:sqdone
+s/@!/'/g
 bfolded
 
 # found here-doc -- swallow it to avoid false hits within its body (but keep
diff --git a/t/chainlint/here-doc-multi-line-string.expect 
b/t/chainlint/here-doc-multi-line-string.expect
index 1e5b724b9d..32038a070c 100644
--- a/t/chainlint/here-doc-multi-line-string.expect
+++ b/t/chainlint/here-doc-multi-line-string.expect
@@ -1,4 +1,4 @@
 (
-?!AMP?!cat && echo multi-line  string"
+?!AMP?!cat && echo "multi-line string"
bap
 >)
diff --git a/t/chainlint/multi-line-string.expect 
b/t/chainlint/multi-line-string.expect
index 8334c4cc8e..170cb59993 100644
--- a/t/chainlint/multi-line-string.expect
+++ b/t/chainlint/multi-line-string.expect
@@ -1,9 +1,15 @@
 (
-   x=line 1line 2  line 3" &&
-?!AMP?!y=line 1line2'
+   x="line 1   line 2  line 3" &&
+?!AMP?!y='line 1   line2'
foobar
 >) &&
 (
echo "there's nothing to see here" &&
exit
+>) &&
+(
+   echo "xyz" "abc def ghi" &&
+   echo 'xyz' 'abc def ghi' &&
+   echo 'xyz' "abc def ghi" &&
+   barfoo
 >)
diff --git a/t/chainlint/multi-line-string.test 
b/t/chainlint/multi-line-string.test
index 14cb44d51c..287ab89705 100644
--- a/t/chainlint/multi-line-string.test
+++ b/t/chainlint/multi-line-string.test
@@ -12,4 +12,16 @@
 # LINT: starting multi-line single-quoted string
echo "there's nothing to see here" &&
exit
+) &&
+(
+   echo "xyz" "abc
+   def
+   ghi" &&
+   echo 'xyz' 'abc
+   def
+   ghi' &&
+   echo 'xyz' "abc
+   def
+   ghi" &&
+   barfoo
 )
-- 
2.18.0.758.g1932418f46

[PATCH 0/5] chainlint: improve robustness against "unusual" shell coding

[Eric Sunshine]

This series improves chainlint's robustness when faced with the sort of
unusual shell coding in contrib/subtree/t7900 which triggered a
false-positive, as reported by Jonathan[1]. Jonathan has already
rewritten[2] that code to be cleaner and more easily understood (and,
consequently, to avoid triggering the false-positive), thus the
improvements in this series are not strictly necessary.

Nevertheless, it seems prudent to make chainlint more robust against
such unusual coding as an aid to future less-experienced test writers,
making it less likely for them to trigger a false-positive and waste
time trying to decipher a non-existent problem (in their code).

In [3], I said that 'sed' couldn't "be coerced" into dealing with nested
here-docs with arbitrary tag names (explaining why it recognized only a
"blessed" set of hard-coded names). However, I put a bit of thought into
it and figured out how to do it. Patch 1/5 is the result.

This applies atop 'master'.

[1]: 
https://public-inbox.org/git/20180730181356.ga156...@aiede.svl.corp.google.com/
[2]: 
https://public-inbox.org/git/20180730190738.gd156...@aiede.svl.corp.google.com/
[3]: 
https://public-inbox.org/git/CAPig+cRTgh6DStUdmXqvhbL_7sQY6wu21h27rjq_i=kz_d+...@mail.gmail.com/

Eric Sunshine (5):
  chainlint: match arbitrary here-docs tags rather than hard-coded names
  chainlint: recognize multi-line $(...) when command cuddled with "$("
  chainlint: let here-doc and multi-line string commence on same line
  chainlint: recognize multi-line quoted strings more robustly
  chainlint: add test of pathological case which triggered false
positive

 t/chainlint.sed   | 98 ---
 t/chainlint/here-doc-close-subshell.expect|  2 +
 t/chainlint/here-doc-close-subshell.test  |  5 +
 .../here-doc-multi-line-command-subst.expect  |  5 +
 .../here-doc-multi-line-command-subst.test|  9 ++
 t/chainlint/here-doc-multi-line-string.expect |  4 +
 t/chainlint/here-doc-multi-line-string.test   |  8 ++
 t/chainlint/here-doc.expect   |  2 +
 t/chainlint/here-doc.test |  7 ++
 ...ti-line-nested-command-substitution.expect | 11 ++-
 ...ulti-line-nested-command-substitution.test | 11 ++-
 t/chainlint/multi-line-string.expect  | 10 +-
 t/chainlint/multi-line-string.test| 12 +++
 t/chainlint/nested-here-doc.expect|  2 +
 t/chainlint/nested-here-doc.test  | 10 ++
 t/chainlint/subshell-here-doc.expect  |  4 +
 t/chainlint/subshell-here-doc.test|  8 ++
 t/chainlint/t7900-subtree.expect  | 10 ++
 t/chainlint/t7900-subtree.test| 22 +
 19 files changed, 199 insertions(+), 41 deletions(-)
 create mode 100644 t/chainlint/here-doc-close-subshell.expect
 create mode 100644 t/chainlint/here-doc-close-subshell.test
 create mode 100644 t/chainlint/here-doc-multi-line-command-subst.expect
 create mode 100644 t/chainlint/here-doc-multi-line-command-subst.test
 create mode 100644 t/chainlint/here-doc-multi-line-string.expect
 create mode 100644 t/chainlint/here-doc-multi-line-string.test
 create mode 100644 t/chainlint/t7900-subtree.expect
 create mode 100644 t/chainlint/t7900-subtree.test

-- 
2.18.0.758.g1932418f46

[PATCH 5/5] chainlint: add test of pathological case which triggered false positive

[Eric Sunshine]

This extract from contrib/subtree/t7900 triggered a false positive due
to three chainlint limitations:

* recognizing only a "blessed" set of here-doc tag names in a subshell
  ("EOF", "EOT", "INPUT_END"), of which "TXT" is not a member

* inability to recognize multi-line $(...) when the first statement of
  the body is cuddled with the opening "$("

* inability to recognize multiple constructs on a single line, such as
  opening a multi-line $(...) and starting a here-doc

Now that all of these shortcomings have been addressed, turn this rather
pathological bit of shell coding into a chainlint test case.

Signed-off-by: Eric Sunshine 
---
 t/chainlint/t7900-subtree.expect | 10 ++
 t/chainlint/t7900-subtree.test   | 22 ++
 2 files changed, 32 insertions(+)
 create mode 100644 t/chainlint/t7900-subtree.expect
 create mode 100644 t/chainlint/t7900-subtree.test

diff --git a/t/chainlint/t7900-subtree.expect b/t/chainlint/t7900-subtree.expect
new file mode 100644
index 00..c9913429e6
--- /dev/null
+++ b/t/chainlint/t7900-subtree.expect
@@ -0,0 +1,10 @@
+(
+   chks="sub1sub2sub3sub4" &&
+   chks_sub=$(cat | sed 's,^,sub dir/,'
+>>) &&
+   chkms="main-sub1main-sub2main-sub3main-sub4" &&
+   chkms_sub=$(cat | sed 's,^,sub dir/,'
+>>) &&
+   subfiles=$(git ls-files) &&
+   check_equal "$subfiles" "$chkms$chks"
+>)
diff --git a/t/chainlint/t7900-subtree.test b/t/chainlint/t7900-subtree.test
new file mode 100644
index 00..277d8358df
--- /dev/null
+++ b/t/chainlint/t7900-subtree.test
@@ -0,0 +1,22 @@
+(
+   chks="sub1
+sub2
+sub3
+sub4" &&
+   chks_sub=$(cat <

[PATCH 2/5] chainlint: recognize multi-line $(...) when command cuddled with "$("

[Eric Sunshine]

For multi-line $(...) expressions nested within subshells, chainlint.sed
only recognizes:

x=$(
echo foo &&
...

but it is not unlikely that test authors may also cuddle the command
with the opening "$(", so support that style, as well:

x=$(echo foo &&
...

The closing ")" is already correctly recognized when cuddled or not.

Signed-off-by: Eric Sunshine 
---
 t/chainlint.sed   |  2 +-
 .../multi-line-nested-command-substitution.expect | 11 ++-
 .../multi-line-nested-command-substitution.test   | 11 ++-
 3 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/t/chainlint.sed b/t/chainlint.sed
index bd76c5d181..a0726d3e7d 100644
--- a/t/chainlint.sed
+++ b/t/chainlint.sed
@@ -216,7 +216,7 @@ s/.*\n//
 # "$(...)" -- command substitution; not closing ")"
 /\$([^)][^)]*)[^)]*$/bcheckchain
 # multi-line "$(...\n...)" -- command substitution; treat as nested subshell
-/\$([  ]*$/bnest
+/\$([^)]*$/bnest
 # "=(...)" -- Bash array assignment; not closing ")"
 /=(/bcheckchain
 # closing "...) &&"
diff --git a/t/chainlint/multi-line-nested-command-substitution.expect 
b/t/chainlint/multi-line-nested-command-substitution.expect
index 19c023b1c8..59b6c8b850 100644
--- a/t/chainlint/multi-line-nested-command-substitution.expect
+++ b/t/chainlint/multi-line-nested-command-substitution.expect
@@ -6,4 +6,13 @@
 >> ) &&
echo ok
 >) |
-sort
+sort &&
+(
+   bar &&
+   x=$(echo bar |
+   cat
+>> ) &&
+   y=$(echo baz |
+>> fip) &&
+   echo fail
+>)
diff --git a/t/chainlint/multi-line-nested-command-substitution.test 
b/t/chainlint/multi-line-nested-command-substitution.test
index ca0620ab6b..300058341b 100644
--- a/t/chainlint/multi-line-nested-command-substitution.test
+++ b/t/chainlint/multi-line-nested-command-substitution.test
@@ -6,4 +6,13 @@
) &&
echo ok
 ) |
-sort
+sort &&
+(
+   bar &&
+   x=$(echo bar |
+   cat
+   ) &&
+   y=$(echo baz |
+   fip) &&
+   echo fail
+)
-- 
2.18.0.758.g1932418f46

[PATCH 3/5] chainlint: let here-doc and multi-line string commence on same line

[Eric Sunshine]

After swallowing a here-doc, chainlint.sed assumes that no other
processing needs to be done on the line aside from checking for &&-chain
breakage; likewise, after folding a multi-line quoted string. However,
it's conceivable (even if unlikely in practice) that both a here-doc and
a multi-line quoted string might commence on the same line:

cat <<\EOF && echo "foo
bar"
data
EOF

Support this case by sending the line (after swallowing and folding)
through the normal processing sequence rather than jumping directly to
the check for broken &&-chain.

This change also allows other somewhat pathological cases to be handled,
such as closing a subshell on the same line starting a here-doc:

(
cat <<-\INPUT)
data
INPUT

or, for instance, opening a multi-line $(...) expression on the same
line starting a here-doc:

x=$(cat <<-\END &&
data
END
echo "x")

among others.

Signed-off-by: Eric Sunshine 
---
 t/chainlint.sed  | 7 ---
 t/chainlint/here-doc-close-subshell.expect   | 2 ++
 t/chainlint/here-doc-close-subshell.test | 5 +
 t/chainlint/here-doc-multi-line-command-subst.expect | 5 +
 t/chainlint/here-doc-multi-line-command-subst.test   | 9 +
 t/chainlint/here-doc-multi-line-string.expect| 4 
 t/chainlint/here-doc-multi-line-string.test  | 8 
 7 files changed, 37 insertions(+), 3 deletions(-)
 create mode 100644 t/chainlint/here-doc-close-subshell.expect
 create mode 100644 t/chainlint/here-doc-close-subshell.test
 create mode 100644 t/chainlint/here-doc-multi-line-command-subst.expect
 create mode 100644 t/chainlint/here-doc-multi-line-command-subst.test
 create mode 100644 t/chainlint/here-doc-multi-line-string.expect
 create mode 100644 t/chainlint/here-doc-multi-line-string.test

diff --git a/t/chainlint.sed b/t/chainlint.sed
index a0726d3e7d..6c891bf383 100644
--- a/t/chainlint.sed
+++ b/t/chainlint.sed
@@ -157,6 +157,7 @@ s/.*\n//
 /^[^']*'[^']*$/{
/"[^'"]*'[^'"]*"/!bsqstring
 }
+:folded
 # here-doc -- swallow it
 /<<[   ]*[-\\]*[A-Z0-9_][A-Z0-9_]*/bheredoc
 # comment or empty line -- discard since final non-comment, non-empty line
@@ -255,7 +256,7 @@ s/"//g
 N
 s/\n//
 /"/!bdqstring
-bcheckchain
+bfolded
 
 # found multi-line single-quoted string '...\n...' -- slurp until end of string
 :sqstring
@@ -263,7 +264,7 @@ s/'//g
 N
 s/\n//
 /'/!bsqstring
-bcheckchain
+bfolded
 
 # found here-doc -- swallow it to avoid false hits within its body (but keep
 # the command to which it was attached)
@@ -278,7 +279,7 @@ N
 }
 s/^<[^>]*>//
 s/\n.*$//
-bcheckchain
+bfolded
 
 # found "case ... in" -- pass through untouched
 :case
diff --git a/t/chainlint/here-doc-close-subshell.expect 
b/t/chainlint/here-doc-close-subshell.expect
new file mode 100644
index 00..f011e335e5
--- /dev/null
+++ b/t/chainlint/here-doc-close-subshell.expect
@@ -0,0 +1,2 @@
+(
+>  cat)
diff --git a/t/chainlint/here-doc-close-subshell.test 
b/t/chainlint/here-doc-close-subshell.test
new file mode 100644
index 00..b857ff5467
--- /dev/null
+++ b/t/chainlint/here-doc-close-subshell.test
@@ -0,0 +1,5 @@
+(
+# LINT: line contains here-doc and closes nested subshell
+   cat <<-\INPUT)
+   fizz
+   INPUT
diff --git a/t/chainlint/here-doc-multi-line-command-subst.expect 
b/t/chainlint/here-doc-multi-line-command-subst.expect
new file mode 100644
index 00..e5fb752d2f
--- /dev/null
+++ b/t/chainlint/here-doc-multi-line-command-subst.expect
@@ -0,0 +1,5 @@
+(
+   x=$(bobble &&
+?!AMP?!>>  wiffle)
+   echo $x
+>)
diff --git a/t/chainlint/here-doc-multi-line-command-subst.test 
b/t/chainlint/here-doc-multi-line-command-subst.test
new file mode 100644
index 00..899bc5de8b
--- /dev/null
+++ b/t/chainlint/here-doc-multi-line-command-subst.test
@@ -0,0 +1,9 @@
+(
+# LINT: line contains here-doc and opens multi-line $(...)
+   x=$(bobble <<-\END &&
+   fossil
+   vegetable
+   END
+   wiffle)
+   echo $x
+)
diff --git a/t/chainlint/here-doc-multi-line-string.expect 
b/t/chainlint/here-doc-multi-line-string.expect
new file mode 100644
index 00..1e5b724b9d
--- /dev/null
+++ b/t/chainlint/here-doc-multi-line-string.expect
@@ -0,0 +1,4 @@
+(
+?!AMP?!cat && echo multi-line  string"
+   bap
+>)
diff --git a/t/chainlint/here-doc-multi-line-string.test 
b/t/chainlint/here-doc-multi-line-string.test
new file mode 100644
index 00..a53edbcc8d
--- /dev/null
+++ b/t/chainlint/here-doc-multi-line-string.test
@@ -0,0 +1,8 @@
+(
+# LINT: line contains here-doc and opens multi-line string
+   cat <<-\TXT && echo "multi-line
+   string"
+   fizzle
+   TXT
+   bap
+)
-- 
2.18.0.758.g1932418f46

[PATCH 1/5] chainlint: match arbitrary here-docs tags rather than hard-coded names

[Eric Sunshine]

chainlint.sed swallows top-level here-docs to avoid being fooled by
content which might look like start-of-subshell. It likewise swallows
here-docs in subshells to avoid marking content lines as breaking the
&&-chain, and to avoid being fooled by content which might look like
end-of-subshell, start-of-nested-subshell, or other specially-recognized
constructs.

At the time of implementation, it was believed that it was not possible
to support arbitrary here-doc tag names since 'sed' provides no way to
stash the opening tag name in a variable for later comparison against a
line signaling end-of-here-doc. Consequently, tag names are hard-coded,
with "EOF" being the only tag recognized at the top-level, and only
"EOF", "EOT", and "INPUT_END" being recognized within subshells. Also,
special care was taken to avoid being confused by here-docs nested
within other here-docs.

In practice, this limited number of hard-coded tag names has been "good
enough" for the 13000+ existing Git test, despite many of those tests
using tags other than the recognized ones, since the bodies of those
here-docs do not contain content which would fool the linter.
Nevertheless, the situation is not ideal since someone writing new
tests, and choosing a name not in the "blessed" set could potentially
trigger a false-positive.

To address this shortcoming, upgrade chainlint.sed to handle arbitrary
here-doc tag names, both at the top-level and within subshells.

Signed-off-by: Eric Sunshine 
---
 t/chainlint.sed  | 57 +---
 t/chainlint/here-doc.expect  |  2 +
 t/chainlint/here-doc.test|  7 
 t/chainlint/nested-here-doc.expect   |  2 +
 t/chainlint/nested-here-doc.test | 10 +
 t/chainlint/subshell-here-doc.expect |  4 ++
 t/chainlint/subshell-here-doc.test   |  8 
 7 files changed, 67 insertions(+), 23 deletions(-)

diff --git a/t/chainlint.sed b/t/chainlint.sed
index 5f0882cb38..bd76c5d181 100644
--- a/t/chainlint.sed
+++ b/t/chainlint.sed
@@ -61,6 +61,22 @@
 # "else", and "fi" in if-then-else likewise must not end with "&&", thus
 # receives similar treatment.
 #
+# Swallowing here-docs with arbitrary tags requires a bit of finesse. When a
+# line such as "cat out".
+# As each subsequent line is read, it is appended to the target line and a
+# (whitespace-loose) back-reference match /^<(.*)>\n\1$/ is attempted to see if
+# the content inside "<...>" matches the entirety of the newly-read line. For
+# instance, if the next line read is "some data", when concatenated with the
+# target line, it becomes "cat >out\nsome data", and a match is attempted
+# to see if "EOF" matches "some data". Since it doesn't, the next line is
+# attempted. When a line consisting of only "EOF" (and possible whitespace) is
+# encountered, it is appended to the target line giving "cat >out\nEOF",
+# in which case the "EOF" inside "<...>" does match the text following the
+# newline, thus the closing here-doc tag has been found. The closing tag line
+# and the "<...>" prefix on the target line are then discarded, leaving just
+# the target line "cat >out".
+#
 # To facilitate regression testing (and manual debugging), a ">" annotation is
 # applied to the line containing ")" which closes a subshell, ">>" to a line
 # closing a nested subshell, and ">>>" to a line closing both at once. This
@@ -78,14 +94,17 @@
 
 # here-doc -- swallow it to avoid false hits within its body (but keep the
 # command to which it was attached)
-/<<[   ]*[-\\]*EOF[]*/ {
-   s/[ ]*<<[   ]*[-\\]*EOF//
-   h
+/<<[   ]*[-\\]*[A-Z0-9_][A-Z0-9_]*/ {
+   s/^\(.*\)<<[]*[-\\]*\([A-Z0-9_][A-Z0-9_]*\)/<\2>\1<]*\)>.*\n[  ]*\1[   ]*$/!{
+   s/\n.*$//
+   bhereslurp
+   }
+   s/^<[^>]*>//
+   s/\n.*$//
 }
 
 # one-liner "(...) &&"
@@ -139,9 +158,7 @@ s/.*\n//
/"[^'"]*'[^'"]*"/!bsqstring
 }
 # here-doc -- swallow it
-/<<[   ]*[-\\]*EOF/bheredoc
-/<<[   ]*[-\\]*EOT/bheredoc
-/<<[   ]*[-\\]*INPUT_END/bheredoc
+/<<[   ]*[-\\]*[A-Z0-9_][A-Z0-9_]*/bheredoc
 # comment or empty line -- discard since final non-comment, non-empty line
 # before closing ")", "done", "elsif", "else", or "fi" will need to be
 # re-visited to drop "suspect" marking since final line of those constructs
@@ -249,23 +266,17 @@ s/\n//
 bcheckchain
 
 # found here-doc -- swallow it to avoid false hits within its body (but keep
-# the command to which it was attached); take care to handle here-docs nested
-# within here-docs by only recognizing closing tag matching outer here-doc
-# opening tag
+# the command to which it was attached)
 :heredoc
-/EOF/{ s/[ ]*<<[   ]*[-\\]*EOF//; s/^/EOF/; }
-/EOT/{ s/[ ]*<<[   ]*[-\\]*EOT//; s/^/EOT/; }
-/INPUT_END/{ s/[   ]*<<[   ]*[-\\]*INPUT_END//; s/^/INPUT_END/; }
+s/^\(.*\)<<[   

Re: [PATCH 0/2] Simple fixes to t7406

[Martin Ågren]

On 6 August 2018 at 17:25, Elijah Newren  wrote:
> Changes since v1:
>   - Simplify multiple tests using diff --name-only instead of diff --raw;
> these tests are only interested in which file was modified anyway.
> (Suggested by Junio)
>   - Avoid putting git commands upstream of a pipe, since we don't run under
> set -o pipefail (Suggested by Eric)
>   - Avoid using test_must_fail for system binaries (Pointed out by
> both Eric and Junio)
>
> Elijah Newren (2):

I'm not sure what's up with the count of 2 vs 3.

>   t7406: simplify by using diff --name-only instead of diff --raw
>   t7406: avoid having git commands upstream of a pipe
>   t7406: make a test_must_fail call fail for the right reason

The subject of the final patch doesn't quite match its content anymore.
The problematic test_must_fail is dropped, so it can no longer fail.
Maybe something like "t7406: fix call that was failing for the wrong
reason", or just s/test_must_fail// in what you have.

Martin

[PATCH 2/2] git-instaweb: fix apache2 config with apache >= 2.4

[Sebastian Kisela]

The generated apache2 config fails with apache >= 2.4.  The error log
states:

AH00136: Server MUST relinquish startup privileges before accepting
connections.  Please ensure mod_unixd or other system security
module is loaded.
AH00016: Configuration Failed

Fix this by loading the unixd module.  This works with older httpd as
well, so no IfVersion conditional is needed.  (Tested with httpd-2.2.15
on CentOS-6.)

Written with assistance of Todd Zullinger 

Signed-off-by: Sebastian Kisela 
---
 git-instaweb.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-instaweb.sh b/git-instaweb.sh
index e42e58528..b1da2c374 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -358,7 +358,7 @@ EOF
break
fi
done
-   for mod in mime dir env log_config authz_core
+   for mod in mime dir env log_config authz_core unixd
do
if test -e $module_path/mod_${mod}.so
then
-- 
2.18.0.99.gd2ee41e0e

[PATCH 1/2] git-instaweb: support Fedora/Red Hat apache module path

[Sebastian Kisela]

On Fedora-derived systems, the apache httpd package installs modules
under /usr/lib{,64}/httpd/modules, depending on whether the system is
32- or 64-bit.  A symlink from /etc/httpd/modules is created which
points to the proper module path.  Use it to support apache on Fedora,
CentOS, and Red Hat systems.

Written with assistance of Todd Zullinger 

Signed-off-by: Sebastian Kisela 
---
 git-instaweb.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/git-instaweb.sh b/git-instaweb.sh
index 47e38f34c..e42e58528 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -332,6 +332,8 @@ apache2_conf () {
module_path="/usr/lib/httpd/modules"
test -d "/usr/lib/apache2/modules" &&
module_path="/usr/lib/apache2/modules"
+   test -d "/etc/httpd/modules" &&
+   module_path="/etc/httpd/modules"
fi
bind=
test x"$local" = xtrue && bind='127.0.0.1:'
-- 
2.18.0.99.gd2ee41e0e

Re: What's cooking in git.git (Aug 2018, #02; Mon, 6)

[Eric Sunshine]

On Mon, Aug 6, 2018 at 11:47 PM Eric Sunshine  wrote:
> On Mon, Aug 6, 2018 at 6:36 PM Junio C Hamano  wrote:
> > * pw/rebase-i-author-script-fix (2018-08-02) 2 commits
> >  - sequencer: fix quoting in write_author_script
> >  - sequencer: handle errors in read_author_ident()
> >  (this branch uses es/rebase-i-author-script-fix.)
> >
> >  Undecided.
> >  Is it the list consensus to favor this "with extra code, read the
> >  script written by bad writer" approach?
>
> Phillip's original "effectively one-liner" backward compatibility[1]
> seemed a reasonable compromise[2] between the choices of no backward
> compatibility and heavyweight backward compatibility of his
> re-roll[3]. His reference[4] to an earlier "one-liner" backward
> compatibility solution given similar circumstances bolstered the case
> for his original approach.

To summarize, I think Phillip is planning to re-roll, going with the
simpler backward-compatibility (though he was waiting an "okay" from
you[2]).

[2]: 
https://public-inbox.org/git/455fafb5-3c92-4348-0c2c-0a4ab62cf...@talktalk.net/

Re: What's cooking in git.git (Aug 2018, #02; Mon, 6)

[Ævar Arnfjörð Bjarmason]

On Mon, Aug 06 2018, Junio C Hamano wrote:

> * ab/newhash-is-sha256 (2018-08-06) 2 commits
>  - doc hash-function-transition: pick SHA-256 as NewHash
>  - doc hash-function-transition: note the lack of a changelog
>
>  Documentation update.
>
>  Will Merge to 'next'.

The tip of gitster/ab/newhash-is-sha256 has a unicode-corrupted
signed-off-by line from me. See
https://github.com/git/git/commit/f855a19d45 not sure how that got
there...