Re: [PATCH/RFC 0/5] Keep all info in command-list.txt in git binary

[Eric Sunshine]

On Mon, Mar 26, 2018 at 12:55 PM, Nguyễn Thái Ngọc Duy
 wrote:
> This is pretty rough but I'd like to see how people feel about this
> first.
>
> I notice we have two places for command classification. One in
> command-list.txt, one in __git_list_porcelain_commands() in
> git-completion.bash. People who are following nd/parseopt-completion
> probably know that I'm try to reduce duplication in this script as
> much as possible, this is another step towards that.
>
> By keeping all information of command-list.txt in git binary, we could
> provide the porcelain list to git-completion.bash via "git
> --list-cmds=porcelain", so we don't neeed a separate command
> classification in git-completion.bash anymore.

I like the direction this series is taking.

> Because we have all command synopsis as a side effect, we could
> now support "git help -a --verbose" which prints something like "git
> help", a command name and a description, but we could do it for _all_
> recognized commands. This could help people look for a command even if
> we don't provide "git appropos".

Nice idea, and you practically get this for free (aside from the the
obvious new code) since generate-cmdlist.sh already plucks the summary
for each command directly from Documentation/git-*.txt.

Re: [PATCH/RFC 3/5] generate-cmdlist.sh: keep all information in common-cmds.h

[Eric Sunshine]

On Mon, Apr 9, 2018 at 12:59 AM, Eric Sunshine  wrote:
> However, I'm concerned that this change may be going in the wrong
> direction. A line in "### command list" section looks like this:
>
> command-name  category [deprecated] [common]
>
> Although we don't currently have any commands marked with tag
> "deprecated", we very well may have some day. More generally, new
> optional or required tags may be added in the future. As such, the
> line format is relatively free-form. Current clients don't even care
> in what order the tags appears (following 'category') nor how many
> tags there are. The new code added by this patch, however, is far less
> flexible and accommodating since it assumes hard-coded columns for the
> tags (and doesn't even take 'deprecated' into account).
>
> The point of the $match file was to be able to extract only lines
> which mentioned one of the "common groups", and the point of the
> 'substnum' transformation was to transform the group name into a group
> number -- both of these operations were done without caring about the
> exact column the "common group" tag occupied.
>
> Obviously, one option for addressing this concern would be to change
> the definition to make the tag columns fixed and non-optional, which
> would allow the simpler implementation used by this patch. Doing so
> may require fixing other consumers of command-list.txt (though, I'm
> pretty sure existing consumers wouldn't be bothered).

I should follow up by saying that, although the current relatively
free-form line definition is nice due to its flexibility, considering
how infrequently command-list.txt is edited and how much more complex
the script is to support that flexibility, changing to a definition in
which tags are required and at fixed columns seems like the pragmatic
thing to do.

Re: [PATCH/RFC 5/5] help: add "-a --verbose" to list all commands with synopsis

[Eric Sunshine]

On Mon, Mar 26, 2018 at 12:55 PM, Nguyễn Thái Ngọc Duy
 wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/help.c b/help.c
> @@ -282,6 +282,67 @@ void list_porcelain_cmds(void)
> +static const char *get_category_name(unsigned int category)
> +{
> +   switch (category) {
> +   case CAT_ancillaryinterrogators: return _("Ancillary interrogators");
> +   case CAT_ancillarymanipulators: return _("Ancillary manipulators");
> +   case CAT_foreignscminterface: return _("Foreign SCM interface");
> +   case CAT_mainporcelain: return _("Main porcelain");
> +   case CAT_plumbinginterrogators: return _("Plumbing interrogators");
> +   case CAT_plumbingmanipulators: return _("Plumbing interrogators");

s/interrogators"/manipulators"/

> +   case CAT_purehelpers: return _("Pure helpers");
> +   case CAT_synchelpers: return _("Sync helpers");
> +   case CAT_synchingrepositories: return _("Synching repositories");
> +   default:
> +   die("BUG: unknown command category %u", category);
> +   }

Re: [PATCH/RFC 3/5] generate-cmdlist.sh: keep all information in common-cmds.h

[Eric Sunshine]

On Mon, Mar 26, 2018 at 12:55 PM, Nguyễn Thái Ngọc Duy
 wrote:
> common-cmds.h is used to extract the list of common commands (by
> group) and a one-line summary of each command. Some information is
> dropped, for example command category or summary of other commands.
> Update generate-cmdlist.sh to keep all the information. The extra info
> will be used shortly.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
> @@ -2,9 +2,10 @@
>  struct cmdname_help {
> -   char name[16];
> +   char name[32];
> char help[80];
> -   unsigned char group;
> +   unsigned int category;
> +   unsigned int group;
>  };
> @@ -23,27 +24,50 @@ sed -n '
> +echo "#define GROUP_NONE 0xff /* no common group */"
> +echo "#define GROUP_ 0xff /* no common group */"

Meh, this "GROUP_" alias of "GROUP_NONE" isn't so nice.

>  n=0
> -substnum=
>  while read grp
>  do
> -   echo "^git-..*[ ]$grp"
> -   substnum="$substnum${substnum:+;}s/[]$grp/$n/"
> +   echo "#define GROUP_$grp $n"
> n=$(($n+1))
> -done <"$grps" >"$match"
> +done <"$grps"

This patch drops all use of the file $match. Earlier in this script,
not seen in the context, are a couple references to $match which ought
to be adjusted to take its retirement into account:

match=match$$.tmp
trap "rm -f '$grps' '$match'" 0 1 2 3 15

However, I'm concerned that this change may be going in the wrong
direction. A line in "### command list" section looks like this:

command-name  category [deprecated] [common]

Although we don't currently have any commands marked with tag
"deprecated", we very well may have some day. More generally, new
optional or required tags may be added in the future. As such, the
line format is relatively free-form. Current clients don't even care
in what order the tags appears (following 'category') nor how many
tags there are. The new code added by this patch, however, is far less
flexible and accommodating since it assumes hard-coded columns for the
tags (and doesn't even take 'deprecated' into account).

The point of the $match file was to be able to extract only lines
which mentioned one of the "common groups", and the point of the
'substnum' transformation was to transform the group name into a group
number -- both of these operations were done without caring about the
exact column the "common group" tag occupied.

Obviously, one option for addressing this concern would be to change
the definition to make the tag columns fixed and non-optional, which
would allow the simpler implementation used by this patch. Doing so
may require fixing other consumers of command-list.txt (though, I'm
pretty sure existing consumers wouldn't be bothered).

(Perl would be an obvious good choice for retaining the current
relatively free-form line definition without having to jump through
hoops in the shell. Unfortunately, though, a Perl dependency in the
build system can be problematic[1].)

[1]: 
https://public-inbox.org/git/1440365469-9928-1-git-send-email-sunsh...@sunshineco.com/

> -printf 'static struct cmdname_help common_cmds[] = {\n'
> -grep -f "$match" "$1" |
> +echo '/*'
> +printf 'static const char *cmd_categories[] = {\n'
> +grep '^git-' "$1" |

This "grep '^git-'" (and those below) misses some commands, such as
"gitk" and "gitweb". Is that intentional? If not, then you'll probably
need to grab lines following "### command list", as is done earlier in
the script. Same comment for the other couple grep's later in the
patch.

> +awk '{print $2;}' |

At one time, Junio expressed concerns[2] about having an 'awk'
dependency in the build system (in fact, with regards to this same
generation process). Whether he still has such concerns is unknown,
but it should be easy enough to avoid it here (and below).

[2]: https://public-inbox.org/git/20150519004356.GA12854@flurp.local/

> +sort |
> +uniq |
> +while read category; do
> +   printf '\t\"'$category'\",\n'
> +done
> +printf '\tNULL\n};\n\n'
> +echo '*/'
> diff --git a/help.c b/help.c
> @@ -190,6 +190,28 @@ void list_commands(unsigned int colopts,
> +static void extract_common_cmds(struct cmdname_help **p_common_cmds,
> +   int *p_nr)
> +{
> +   int i, nr = 0;
> +   struct cmdname_help *common_cmds;
> +
> +   ALLOC_ARRAY(common_cmds, ARRAY_SIZE(command_list));
> +
> +   for (i = 0; i < ARRAY_SIZE(command_list); i++) {
> +   const struct cmdname_help *cmd = command_list + i;
> +
> +   if (cmd->category != CAT_mainporcelain ||
> +   cmd->group == GROUP_NONE)
> +   continue;

Is the CAT_mainporcelain condition necessary? Before this patch, the
command list would contain only commands with an associated group, so
it seems that you could get by just with the GROUP_NONE condition.

> +
> +   common_cmds[nr++] = *cmd;
> +   }
> +
> +   

Re: [PATCH 1/1] perl: fix installing modules from contrib

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

>>
>> +perllibdir:
>> +@echo $(perllibdir_SQ)

This use of _SQ variant is fishy, isn't it?  Judging from the output
of 

$ git grep _SQ Makefile

e.g.

Makefile:   $(INSTALL) $^ '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
Makefile:   $(INSTALL) $^ '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
Makefile:   $(INSTALL) $^ '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'

I'd expect that any _SQ variant must be referenced inside a single
quote pair.  In fact, that is why a single quote (and nothing else)
in the base variable is replaced with the magic "'\''" sequence,
first stepping out of the current sq context, append a single sq
(escaped with a backslash from the shell), and then stepping back
into another sq context.

I think nobody saw breakage only because they do not have two
consecutive SPs (or any single quote) in their path to $perllibdir.
If we depend on such limitation, there is no point using _SQ
variant, but we already have _SQ variant, let's use it correctly.

Re: [PATCH 3/3] ref-filter: factor ref_array pushing into its own function

[Jeff King]

On Mon, Apr 09, 2018 at 08:18:35AM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > In preparation for callers constructing their own ref_array
> > structs, let's move our own internal push operation into its
> > own function.
> >
> > While we're at it, we can replace REALLOC_ARRAY() with
> > ALLOC_GROW(), which should give the growth operation
> > amortized linear complexity (as opposed to growing by one,
> > which is potentially quadratic, though in-place realloc
> > growth often makes this faster in practice).
> 
> Sorry, but I do not quite get the last part this paragraph.  Up to
> but not including ", though in-place..." I would understand as:
> 
>  - With ALLOC_GROW(), we are explicit in that we are amortizing
>the allocation cost by growing in exponential batches.
> 
>  - With REALLOC_ARRAY(), we are telling the underlying
>realloc(3) that it is OK to pretend that we grow by one, but
>if that permission is literally followed, it could end up
>quadratic.
> 
> So if you have really a bad realloc(3) implementation, switching
> to use ALLOC_GROW() is an improvement.

Yes, that's the gist of what I was saying. Though it is not even
necessarily "a bad realloc". At some point you may hit heap segmentation
and need to copy (though I guess if that happens repeatedly then maybe
your realloc really _is_ bad ;) ).

> But after that I am not sure what you are getting at.  Do you mean
> "In practice, a well-done realloc(3) does the amortizing internally
> anyway, which makes it faster than the grow-by-one quadratic, so
> this change may not make that much of a difference"?  Or do you mean
> "In practice, a well-done realloc(3) internally amortizes better
> than we do manually, so this change may hurt performance"?

The first. I couldn't measure any speedup on glibc, which makes me think
it's mostly doing in-place expansion.

> In either case, I think this splitting into a helper is obviously a
> good move, and using ALLOC_GROW() is conceptually cleaner, as we use
> it everywhere and tend to avoid realloc(3) just to add one item.
> 
> Other than that small confusion (not even a nit), three patches were
> pleasant read.  Thanks.

Thanks. Please feel free to expand or clarify the commit message if that
helps.

-Peff

Re: [PATCH/RFC 2/5] git.c: implement --list-cmds=all and use it in git-completion.bash

[Eric Sunshine]

On Mon, Mar 26, 2018 at 12:55 PM, Nguyễn Thái Ngọc Duy
 wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/help.c b/help.c
> @@ -228,6 +228,21 @@ void list_common_cmds_help(void)
> +void list_all_cmds(void)
> +{
> +   struct cmdnames main_cmds, other_cmds;
> +   int i;
> +
> +   memset(_cmds, 0, sizeof(main_cmds));
> +   memset(_cmds, 0, sizeof(other_cmds));
> +   load_command_list("git-", _cmds, _cmds);
> +
> +   for (i = 0; i < main_cmds.cnt; i++)
> +   puts(main_cmds.names[i]->name);
> +   for (i = 0; i < other_cmds.cnt; i++)
> +   puts(other_cmds.names[i]->name);

clean_cmdnames(_cmds);
clean_cmdnames(_cmds);

> +}

Re: [PATCH] t/helper: 'test-chmtime (--get|-g)' to print only the mtime

[Junio C Hamano]

Paul-Sebastian Ungureanu  writes:

> - find .git/rr-cache/ -type f | xargs test-chmtime -172800 &&
> + test-chmtime -172800 $(find .git/rr-cache/ -type f) &&

You've sneaked this kind of rewrite in, as if you are testing to see
how careful the reviewers are ;-).

We often use "find piped to xargs" pattern to avoid unbounded number
of paths from appearing on a command line, busting platform limits.
In the case of these test scripts, it does not matter very much, but
such a "we can save a process and a pipe this way" optimization is
not within the scope of "chmtime +v often is piped to sed only to
strip path---let's give it a way to just grab the timestamp" topic.
Not a very welcome change.

Having said that, I do not want this to be rerolled if this
unrelated "removal of find-piped-to-xargs pattern" is the only
niggle in the patch, as I've already checked if these conversions
are safe (they are---we are not dealing with hundreds of stuff in
.git/objects/ or .git/rr-cache/).  There were conflicts caused by
this patch and nd/combined-test-helper (which makes the test-chmtime
standalone binary as a subcommand of test-tool); I think I resolved
them correctly, but please double check when I update the public
repositories in several hours.

Thanks.

Re: [PATCH] t/helper: 'test-chmtime (--get|-g)' to print only the mtime

[Junio C Hamano]

Paul-Sebastian Ungureanu  writes:

> Compared to 'test-chmtime -v +0 file' which prints the mtime and
> and the file name, 'test-chmtime --get file' displays only the mtime.
> If it is used in combination with (+|=|=+|=-|-)seconds, it changes
> and prints the new value.
>
>   test-chmtime -v +0 file | sed 's/[^0-9].*$//'
>
> is now equivalent to:
>
>   test-chmtime --get file
>
> Signed-off-by: Paul-Sebastian Ungureanu 
> ---
>  t/helper/test-chmtime.c  | 44 +++-
>  t/t2022-checkout-paths.sh|  4 +--
>  t/t3404-rebase-interactive.sh|  2 +-
>  t/t3510-cherry-pick-sequence.sh  |  4 +--
>  t/t4200-rerere.sh|  8 ++---
>  t/t5000-tar-tree.sh  |  2 +-
>  t/t6022-merge-rename.sh  | 25 +++-
>  t/t6501-freshen-objects.sh   |  6 ++--
>  t/t7508-status.sh|  4 +--
>  t/t7701-repack-unpack-unreachable.sh |  6 ++--
>  10 files changed, 63 insertions(+), 42 deletions(-)

Thanks both for suggesting and implementing an obvious improvement
and updating many places that are helped by it.

Will queue.
.

Re: [PATCH v12 4/4] ls-remote: create '--sort' option

[Eric Sunshine]

On Sun, Apr 8, 2018 at 6:16 PM, Junio C Hamano  wrote:
> Harald Nordgren  writes:
>> +--sort=::
>> + Sort based on the key given. [...]
>
> This is a tangent but I suspect that the description of --sort in
> git-for-each-ref documentation is wrong on the use of multiple
> options, or I am misreading parse_opt_ref_sorting(), which I think
> appends later keys to the end of the list, and compare_refs() which
> I think yields results when an earlier key in the last decides two
> are not equal and ignores the later keys.  The commit that added the
> description was c0f6dc9b ("git-for-each-ref.txt: minor
> improvements", 2008-06-05), and I think even back then the code in
> builtin-for-each-ref.c appended later keys at the end, and it seems
> nobody complained, so it is possible I am not reading the code right
> before fully adjusting to timezone change ;-)

I just re-read parse_opt_ref_sorting(), and I'm pretty sure that it is
_prepending_ keys to the list, despite the confusingly named variable
"sorting_tail", thus it agrees with the documentation that
--sort= specified last becomes primary sort key.

For me, at least, that behavior (last --sort= becomes primary key)
feels counterintuitive, but it's too late to change it now.

Re: [PATCH] Fix condition for redirecting stderr

[Todd Zullinger]

Lucas Werkmeister wrote:
> Since the --log-destination option was added in 0c591cacb ("daemon: add
> --log-destination=(stderr|syslog|none)", 2018-02-04) with the explicit
> goal of allowing logging to stderr when running in inetd mode, we should
> not always redirect stderr to /dev/null in inetd mode, but rather only
> when stderr is not being used for logging.

Perhaps 's/^F/daemon: f/' on the subject?  (Junio may well
already have done so while queueing locally.)

The patch itself looks reasonable (to my relatively untrained eyes).

-- 
Todd
~~
Hardware:  the parts of a computer that can be kicked.
-- Jeff Pesis

Re: [PATCH] t5404: relax overzealous test

[Junio C Hamano]

Jeff King  writes:

> On Fri, Apr 06, 2018 at 09:31:22PM +0200, Johannes Schindelin wrote:
>
>> In 0b294c0abf0 (make deleting a missing ref more quiet, 2008-07-08), we
>> added a test to verify that deleting an already-deleted ref does not
>> show an error.
>
> Amazing that it took this long to come up.
> ...
>> This patch chooses instead to look for the prefix "error:" at the
>> beginning of the line, so that there can be no ambiguity that any catch
>> was indeed a message generated by Git's `error_builtin()` function.
>
> Yep, this seems obviously correct.

Hits in

$ git grep 'grep ["'\'']*error' t

shows that many checks for errors that are not anchored, but I do
not think any of them are looking for the string in a pathname other
than this instance, so it would be OK.

Thanks, both.  Will apply.

Re: [PATCH] git-svn: avoid warning on undef readline()

[Junio C Hamano]

Eric Wong  writes:

> Ævar Arnfjörð Bjarmason  wrote:
>> See https://public-inbox.org/git/86h8oobl36@phe.ftfl.ca/ for the
>> original report.
>
> Thanks for taking a look at this.  Also https://bugs.debian.org/894997
>
>> --- a/perl/Git.pm
>> +++ b/perl/Git.pm
>> @@ -554,7 +554,7 @@ sub get_record {
>>  my ($fh, $rs) = @_;
>>  local $/ = $rs;
>>  my $rec = <$fh>;
>> -chomp $rec if defined $rs;
>> +chomp $rec if defined $rs and defined $rec;
>
> I'm struggling to understand the reason for the "defined $rs"
> check.  I think it was a braino on my part and meant to use:
>
>   chomp $rec if defined $rec;

That sounds quite plausible, so if there is no further discussion,
let me queue a version that does s/rs/rec/ on that line myself
(bypassing a pull from your repository at bogomips.org/).

Thanks.

Re: [RFC PATCH 4/7] dir: Directories should be checked for matching pathspecs too

[Junio C Hamano]

Jeff King  writes:

> To be honest, I don't know. Most of dir.c predates me, and I've tried to
> avoid looking at it too hard. But I had a vague recollection of it being
> "best effort", and this bit from cf424f5fd89b reinforces that:
>
>   However, read_directory does not actually check against our pathspec.
>   It uses a simplified version that may turn up false positives. As a
>   result, we need to check that any hits match our pathspec.

At least the original design of the traversal was "try making use of
pathspec during the traversal when we can cheaply filter out obvious
non-hits and avoid recursing into an entire hierarchy---false negative
is an absolute no-no, but forcing the consumer to post filter is OK".

> ... But I think that anybody who looks at the output of
> fill_directory() does need to be aware that they may get more entries
> than they expected, and has to apply the pathspecs themselves.

That matches with my understanding of how "fill" thing worked from
early days.

Re: [PATCH v13 4/4] ls-remote: create '--sort' option

[Harald Nordgren]

On Mon, Apr 9, 2018 at 2:56 AM, Junio C Hamano  wrote:
>
> With the update since v12, I think "because `ls-remote` deals only
> with remotes," can be dropped entirely, and still convey what needs
> to be told: "Be aware some keys that needs access to objects that
> are not here won't work".
>
> Instead, it is probably a better idea to spend that half-line worth
> of characters to describe in what way they do not work (do they try
> to deref a NULL pointer and dump core?  do they notice we need
> missing objects and give an error?).

Updated the docs again. The specific error we get on commiterdate is

fatal: missing object f0d0fd3a5985d5e588da1e1d11c85fba0ae132f8 for
refs/pull/10/head

So no core dump. But a fatal error with a possibly confusing message.

[PATCH v14 2/4] ref-filter: make ref_array_item allocation more consistent

[Harald Nordgren]

From: Jeff King 

We have a helper function to allocate ref_array_item
structs, but it only takes a subset of the possible fields
in the struct as initializers. We could have it accept an
argument for _every_ field, but that becomes a pain for the
fields which some callers don't want to set initially.

Instead, let's be explicit that it takes only the minimum
required to create the ref, and that callers should then
fill in the rest themselves.

Signed-off-by: Jeff King 
Signed-off-by: Harald Nordgren 
---
 ref-filter.c | 18 +++---
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index ade97a848..c1c3cc948 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1824,15 +1824,18 @@ static const struct object_id *match_points_at(struct 
oid_array *points_at,
return NULL;
 }
 
-/* Allocate space for a new ref_array_item and copy the objectname and flag to 
it */
+/*
+ * Allocate space for a new ref_array_item and copy the name and oid to it.
+ *
+ * Callers can then fill in other struct members at their leisure.
+ */
 static struct ref_array_item *new_ref_array_item(const char *refname,
-const struct object_id *oid,
-int flag)
+const struct object_id *oid)
 {
struct ref_array_item *ref;
+
FLEX_ALLOC_STR(ref, refname, refname);
oidcpy(>objectname, oid);
-   ref->flag = flag;
 
return ref;
 }
@@ -1927,12 +1930,13 @@ static int ref_filter_handler(const char *refname, 
const struct object_id *oid,
 * to do its job and the resulting list may yet to be pruned
 * by maxcount logic.
 */
-   ref = new_ref_array_item(refname, oid, flag);
+   ref = new_ref_array_item(refname, oid);
ref->commit = commit;
+   ref->flag = flag;
+   ref->kind = kind;
 
REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
-   ref->kind = kind;
return 0;
 }
 
@@ -2169,7 +2173,7 @@ void pretty_print_ref(const char *name, const struct 
object_id *oid,
  const struct ref_format *format)
 {
struct ref_array_item *ref_item;
-   ref_item = new_ref_array_item(name, oid, 0);
+   ref_item = new_ref_array_item(name, oid);
ref_item->kind = ref_kind_from_refname(name);
show_ref_array_item(ref_item, format);
free_array_item(ref_item);
-- 
2.14.3 (Apple Git-98)

[PATCH v14 4/4] ls-remote: create '--sort' option

[Harald Nordgren]

Create a '--sort' option for ls-remote, based on the one from
for-each-ref. This e.g. allows ref names to be sorted by version
semantics, so that v1.2 is sorted before v1.10.

Signed-off-by: Harald Nordgren 
---

Notes:
Changes according to Junio Hamano's code review (2)

 Documentation/git-ls-remote.txt | 16 +++-
 builtin/ls-remote.c | 28 ++---
 t/t5512-ls-remote.sh| 54 +++--
 3 files changed, 87 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index 5f2628c8f..6ad1e34af 100644
--- a/Documentation/git-ls-remote.txt
+++ b/Documentation/git-ls-remote.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 
 [verse]
 'git ls-remote' [--heads] [--tags] [--refs] [--upload-pack=]
- [-q | --quiet] [--exit-code] [--get-url]
+ [-q | --quiet] [--exit-code] [--get-url] [--sort=]
  [--symref] [ [...]]
 
 DESCRIPTION
@@ -60,6 +60,16 @@ OPTIONS
upload-pack only shows the symref HEAD, so it will be the only
one shown by ls-remote.
 
+--sort=::
+   Sort based on the key given. Prefix `-` to sort in descending order
+   of the value. Supports "version:refname" or "v:refname" (tag names
+   are treated as versions). The "version:refname" sort order can also
+   be affected by the "versionsort.suffix" configuration variable.
+   See linkgit:git-for-each-ref[1] for more sort options, but be aware
+   keys like `committerdate` that require access to the objects
+   themselves will not work for refs whose objects have not yet been
+   fetched from the remote, and will give a `missing object` error.
+
 ::
The "remote" repository to query.  This parameter can be
either a URL or the name of a remote (see the GIT URLS and
@@ -90,6 +100,10 @@ EXAMPLES
c5db5456ae3b0873fc659c19fafdde22313cc441refs/tags/v0.99.2
7ceca275d047c90c0c7d5afb13ab97efdf51bd6erefs/tags/v0.99.3
 
+SEE ALSO
+
+linkgit:git-check-ref-format[1].
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 540d56429..b26c53670 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -1,6 +1,7 @@
 #include "builtin.h"
 #include "cache.h"
 #include "transport.h"
+#include "ref-filter.h"
 #include "remote.h"
 
 static const char * const ls_remote_usage[] = {
@@ -43,10 +44,13 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
int show_symref_target = 0;
const char *uploadpack = NULL;
const char **pattern = NULL;
+   int i;
 
struct remote *remote;
struct transport *transport;
const struct ref *ref;
+   struct ref_array ref_array;
+   static struct ref_sorting *sorting = NULL, **sorting_tail = 
 
struct option options[] = {
OPT__QUIET(, N_("do not print remote URL")),
@@ -60,6 +64,8 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
OPT_BIT(0, "refs", , N_("do not show peeled tags"), 
REF_NORMAL),
OPT_BOOL(0, "get-url", _url,
 N_("take url..insteadOf into account")),
+   OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
+N_("field name to sort on"), 
_opt_ref_sorting),
OPT_SET_INT_F(0, "exit-code", ,
  N_("exit with exit code 2 if no matching refs are 
found"),
  2, PARSE_OPT_NOCOMPLETE),
@@ -68,6 +74,8 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
OPT_END()
};
 
+   memset(_array, 0, sizeof(ref_array));
+
argc = parse_options(argc, argv, prefix, options, ls_remote_usage,
 PARSE_OPT_STOP_AT_NON_OPTION);
dest = argv[0];
@@ -90,6 +98,7 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
 
if (get_url) {
printf("%s\n", *remote->url);
+   UNLEAK(sorting);
return 0;
}
 
@@ -98,20 +107,33 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
transport_set_option(transport, TRANS_OPT_UPLOADPACK, 
uploadpack);
 
ref = transport_get_remote_refs(transport);
-   if (transport_disconnect(transport))
+   if (transport_disconnect(transport)) {
+   UNLEAK(sorting);
return 1;
+   }
 
if (!dest && !quiet)
fprintf(stderr, "From %s\n", *remote->url);
for ( ; ref; ref = ref->next) {
+   struct ref_array_item *item;
if (!check_ref_type(ref, flags))
continue;
if (!tail_match(pattern, ref->name))
continue;
+   item = ref_array_push(_array, ref->name, >old_oid);
+   

[PATCH v14 1/4] ref-filter: use "struct object_id" consistently

[Harald Nordgren]

From: Jeff King 

Internally we store a "struct object_id", and all of our
callers have one to pass us. But we insist that they peel it
to its bare-sha1 hash, which we then hashcpy() into place.
Let's pass it around as an object_id, which future-proofs us
for a post-sha1 world.

Signed-off-by: Jeff King 
Signed-off-by: Harald Nordgren 
---
 builtin/tag.c|  2 +-
 builtin/verify-tag.c |  2 +-
 ref-filter.c | 10 +-
 ref-filter.h |  2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index da186691e..42278f516 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -117,7 +117,7 @@ static int verify_tag(const char *name, const char *ref,
return -1;
 
if (format->format)
-   pretty_print_ref(name, oid->hash, format);
+   pretty_print_ref(name, oid, format);
 
return 0;
 }
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index ad7b79fa5..6fa04b751 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -72,7 +72,7 @@ int cmd_verify_tag(int argc, const char **argv, const char 
*prefix)
}
 
if (format.format)
-   pretty_print_ref(name, oid.hash, );
+   pretty_print_ref(name, , );
}
return had_error;
 }
diff --git a/ref-filter.c b/ref-filter.c
index 45fc56216..ade97a848 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1826,12 +1826,12 @@ static const struct object_id *match_points_at(struct 
oid_array *points_at,
 
 /* Allocate space for a new ref_array_item and copy the objectname and flag to 
it */
 static struct ref_array_item *new_ref_array_item(const char *refname,
-const unsigned char 
*objectname,
+const struct object_id *oid,
 int flag)
 {
struct ref_array_item *ref;
FLEX_ALLOC_STR(ref, refname, refname);
-   hashcpy(ref->objectname.hash, objectname);
+   oidcpy(>objectname, oid);
ref->flag = flag;
 
return ref;
@@ -1927,7 +1927,7 @@ static int ref_filter_handler(const char *refname, const 
struct object_id *oid,
 * to do its job and the resulting list may yet to be pruned
 * by maxcount logic.
 */
-   ref = new_ref_array_item(refname, oid->hash, flag);
+   ref = new_ref_array_item(refname, oid, flag);
ref->commit = commit;
 
REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
@@ -2165,11 +2165,11 @@ void show_ref_array_item(struct ref_array_item *info,
putchar('\n');
 }
 
-void pretty_print_ref(const char *name, const unsigned char *sha1,
+void pretty_print_ref(const char *name, const struct object_id *oid,
  const struct ref_format *format)
 {
struct ref_array_item *ref_item;
-   ref_item = new_ref_array_item(name, sha1, 0);
+   ref_item = new_ref_array_item(name, oid, 0);
ref_item->kind = ref_kind_from_refname(name);
show_ref_array_item(ref_item, format);
free_array_item(ref_item);
diff --git a/ref-filter.h b/ref-filter.h
index 0d98342b3..68268f9eb 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -132,7 +132,7 @@ void setup_ref_filter_porcelain_msg(void);
  * Print a single ref, outside of any ref-filter. Note that the
  * name must be a fully qualified refname.
  */
-void pretty_print_ref(const char *name, const unsigned char *sha1,
+void pretty_print_ref(const char *name, const struct object_id *oid,
  const struct ref_format *format);
 
 #endif /*  REF_FILTER_H  */
-- 
2.14.3 (Apple Git-98)

[PATCH v14 3/4] ref-filter: factor ref_array pushing into its own function

[Harald Nordgren]

From: Jeff King 

In preparation for callers constructing their own ref_array
structs, let's move our own internal push operation into its
own function.

While we're at it, we can replace REALLOC_ARRAY() with
ALLOC_GROW(), which should give the growth operation
amortized linear complexity (as opposed to growing by one,
which is potentially quadratic, though in-place realloc
growth often makes this faster in practice).

Signed-off-by: Jeff King 
Signed-off-by: Harald Nordgren 
---
 ref-filter.c | 16 +---
 ref-filter.h |  8 
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index c1c3cc948..6e9328b27 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1840,6 +1840,18 @@ static struct ref_array_item *new_ref_array_item(const 
char *refname,
return ref;
 }
 
+struct ref_array_item *ref_array_push(struct ref_array *array,
+ const char *refname,
+ const struct object_id *oid)
+{
+   struct ref_array_item *ref = new_ref_array_item(refname, oid);
+
+   ALLOC_GROW(array->items, array->nr + 1, array->alloc);
+   array->items[array->nr++] = ref;
+
+   return ref;
+}
+
 static int ref_kind_from_refname(const char *refname)
 {
unsigned int i;
@@ -1930,13 +1942,11 @@ static int ref_filter_handler(const char *refname, 
const struct object_id *oid,
 * to do its job and the resulting list may yet to be pruned
 * by maxcount logic.
 */
-   ref = new_ref_array_item(refname, oid);
+   ref = ref_array_push(ref_cbdata->array, refname, oid);
ref->commit = commit;
ref->flag = flag;
ref->kind = kind;
 
-   REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
-   ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
return 0;
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index 68268f9eb..76cf87cb6 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -135,4 +135,12 @@ void setup_ref_filter_porcelain_msg(void);
 void pretty_print_ref(const char *name, const struct object_id *oid,
  const struct ref_format *format);
 
+/*
+ * Push a single ref onto the array; this can be used to construct your own
+ * ref_array without using filter_refs().
+ */
+struct ref_array_item *ref_array_push(struct ref_array *array,
+ const char *refname,
+ const struct object_id *oid);
+
 #endif /*  REF_FILTER_H  */
-- 
2.14.3 (Apple Git-98)

Re: [PATCH v13 4/4] ls-remote: create '--sort' option

[Junio C Hamano]

Harald Nordgren  writes:

> Create a '--sort' option for ls-remote, based on the one from
> for-each-ref. This e.g. allows ref names to be sorted by version
> semantics, so that v1.2 is sorted before v1.10.
>
> Signed-off-by: Harald Nordgren 
> ---

Thanks.

> +--sort=::
> + Sort based on the key given. Prefix `-` to sort in descending order
> + of the value. Supports "version:refname" or "v:refname" (tag names
> + are treated as versions). The "version:refname" sort order can also
> + be affected by the "versionsort.suffix" configuration variable.
> + See linkgit:git-for-each-ref[1] for more sort options, but be aware
> + that because `ls-remote` deals only with remotes, keys like
> + `committerdate` that requires access to the objects themselves will
> + not work for refs whose objects have not yet been fetched from the
> + remote.

With the update since v12, I think "because `ls-remote` deals only
with remotes," can be dropped entirely, and still convey what needs
to be told: "Be aware some keys that needs access to objects that
are not here won't work".

Instead, it is probably a better idea to spend that half-line worth
of characters to describe in what way they do not work (do they try
to deref a NULL pointer and dump core?  do they notice we need
missing objects and give an error?).

> diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
> index 540d56429..b5ca67167 100644
> --- a/builtin/ls-remote.c
> +++ b/builtin/ls-remote.c

As I said earlier, let's keep these extra UNLEAK() near early
return; they point developers at the places that needs future work.

Thanks.



diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index b5ca67167d..d3851074c2 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -98,6 +98,7 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
 
if (get_url) {
printf("%s\n", *remote->url);
+   UNLEAK(sorting);
return 0;
}
 
@@ -106,8 +107,10 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
transport_set_option(transport, TRANS_OPT_UPLOADPACK, 
uploadpack);
 
ref = transport_get_remote_refs(transport);
-   if (transport_disconnect(transport))
+   if (transport_disconnect(transport)) {
+   UNLEAK(sorting);
return 1;
+   }
 
if (!dest && !quiet)
fprintf(stderr, "From %s\n", *remote->url);

Re: [PATCH v12 4/4] ls-remote: create '--sort' option

[Junio C Hamano]

Harald Nordgren  writes:

>> It is not too big a deal, but I find that liberal sprinkling of
>> UNLEAK() is somewhat unsightly.  From the code we leave with this
>> change, it is clear what we'd need to do when we make the code fully
>> leak-proof (i.e. we'd have a several lines to free resources held by
>> these two variables here, and then UNLEAK() that appear earlier in
>> the function will become a "goto cleanup;" after setting appropriate
>> value to "status"), so let's not worry about it.
>
> Removed the "extra" unleak annotations within the if-return blocks,
> but kept them at the end.

I actually think that is worse.  The UNLEAK()s near early-return
serve to remind us: "here we are making early return and when we
properly plug the leak, here is one of the places we need to fix".
Please keep them, unless (and I do not recommend to do this) you are
plugging the leaks for real.

Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches

[Eric Sunshine]

On Sun, Apr 8, 2018 at 10:24 AM, Thomas Gummerer  wrote:
> On 04/08, Eric Sunshine wrote:
>> On Sat, Mar 31, 2018 at 11:17 AM, Thomas Gummerer  
>> wrote:
> Let me think through some of the cases here, of 'git worktre add
>  ' with various flags and what the UI would be with
> that added:
>
>   - no flags:
>
> $ git worktree add ../test origin/master
> Checking out 'origin/master'
> Checking out files: ...%
> New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'
>
>   - -b branch:
>
> $ git worktree add -b test ../test origin/master
> Creating branch 'test'
> Checking out 'origin/master'

Did you mean "Checking out 'test'"?

> Checking out files: ...%
> New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'
>
> Would we want to omit the "Checking out ..." here?  I'm leaning
> towards yes, but dunno?

To which "Checking out" message do you refer, the one showing the
branch name or the one showing the checkout progress?

I'd probably agree that showing both "Creating" and "Checkout out" is
overkill. However, see my response[1] to your "fixup!" patch in which
I explore the idea that unifying "Checking out 'branch' and "Creating
branch" messages may be a good idea and get us out of some UI jams
which seem to be cropping up.

[1]: 
https://public-inbox.org/git/20180325134947.25828-1-t.gumme...@gmail.com/T/#m5d38b0c6427609e8c36aa6af83d518791c1e1581

>   - Original dwim with --detach flag
>
> $ git worktree add --detach ../test
> Checking out 'c2a499e6c'
> Checking out files: ...%
> New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'
>
> Looking at this, I'm not sure what's best here.  I'm not sure I'm a
> fan of the duplicate "Checking out " message (I assume that's what you
> meant above, or did you mean just "Checkout ..."?)

Taking [1] into account, this might become something like:

$ git worktree add --detach ../test
Preparing worktree (detached HEAD c2a499e6c)
Checking out files: ...%
New worktree HEAD is now at c2a499e6c Gobbledygook

> I als don't think it gives too much context compared to just "Checking
> out files: ...%".  I think it gives a bit more context when that
> message is not displayed at all, as it shows whether files are checked
> out or not, but if we do that, when we create a new branch, the amount
> of output we'd display is getting a bit long, to the point where I
> suspect users would just not read it anymore.
>
> So I personally don't feel like this is worth it, even though it may
> give some context in some cases.

Fair enough observation. The idea suggested in [1] may keep output to
a reasonable amount.

Re: [PATCH v6 6.5/6] fixup! worktree: teach "add" to check out existing branches

[Eric Sunshine]

On Sun, Apr 1, 2018 at 9:11 AM, Thomas Gummerer  wrote:
> So while playing with it a bit more I found one case where the new UI
> is not ideal and a bit confusing.  Namely when the new check out dwim
> kicks in, but there is already a file/directory at the path we're
> giving to 'git worktree add'.
>
> In that case something like the following would be printed:
>
> $ g worktree add ../next
> Checking out branch 'next'
> fatal: '../next' already exists
>
> Instead I think we'd just want the error without the "Checking out
> branch" message, which is what this fixup here does.

Doesn't the same UI "problem" exist when it creates a new branch?

$ git worktree add ../dwim
Creating branch 'dwim'
fatal: '../dwim' already exists

As you mention below, we don't (yet) clean up the newly-created branch
upon failure, so we can't suppress the "Creating branch" message as
you suppress the "Checking out branch" message above (since the user
needs to know that the new branch exists).

This is making me wonder if "Checking out branch" is perhaps the wrong
terminology. What if it said something like this instead:

$ git worktree add ../next
Preparing worktree (branch 'next' <= 'origin/next')
fatal: '../next' already exists

$ git worktree add ../gobble
Preparing worktree (new branch 'gobble')
fatal: '../gobble' already exists

This way, we don't need the special case added by this "fixup!" patch.
(I'm not wedded to the "Preparing" message but just used it as an
example; better suggestions welcome.)

> One thing that gets a bit strange is that the "Checking out branch"
> message and the "Creating branch" messages are printed from different
> places.  But without doing quite some refactoring I don't think
> there's a good way to do that, and I think having the UI do the right
> thing is more important.

The implementation is getting rather ugly, though, especially with
these messages being printed by different bits of code like this.
worktree.c:add_worktree() was supposed to be the low-level worker; it
wasn't intended for it to take on UI duties like this "fixup!" makes
it do. UI should be handled by worktree.c:add().

Taking the above "Preparing..." idea into consideration, then it
should be possible to sidestep this implementation ugliness, I would
think.

> One thing I also noticed is that if a branch is created by 'git
> worktree add', but we fail, we never clean up that branch again, which
> I'm not sure is ideal.  As a pre-existing problem I'd like to keep
> fixing that out of the scope of this series though (at least after
> this series the user would get some output showing that this happened,
> even when the branch is not set up to track a remote), so I'd like to
> keep fixing that out of the scope of this series.

Nice idea, but outside the scope of this series, as you mention.

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -27,6 +27,7 @@ struct add_opts {
> int keep_locked;
> +   int checkout_existing_branch;
>  };
> @@ -316,6 +317,8 @@ static int add_worktree(const char *path, const char 
> *refname,
> +   if (opts->checkout_existing_branch)
> + fprintf_ln(stderr, _("Checking out branch '%s'"), refname);
> if (opts->checkout) {

I'd have expected to see the "if (opts->checkout_existing_branch)
fprintf_ln(...)" inside the following "if (opts->checkout)"
conditional, though, as noted above, I'm not entirely happy about
worktree.c:add_worktree() taking on UI duties.

> cp.argv = NULL;
> argv_array_clear();

Re: [PATCH v12 4/4] ls-remote: create '--sort' option

[Harald Nordgren]

On Mon, Apr 9, 2018 at 12:16 AM, Junio C Hamano  wrote:
>
> I can connect "because it deals only with remotes" and "access to
> the object may fail", but I wonder if we can clarify the former a
> bit better to make it easier to understand for those who are not yet
> familiar with Git.
>
> Keys like `committerdate` that require access to the object will
> not work [***HOW??? Do we get a segfault or something???***] for
> refs whose objects have not yet been fetched from the remote.
>
> I added "for refs whose objects have not yet been fetched", whose
> explicit-ness made "will not work" to be an OK expression, but
> without it I would have suggested to rephrase it to "may not work".
>
> This is a tangent but I suspect that the description of --sort in
> git-for-each-ref documentation is wrong on the use of multiple
> options, or I am misreading parse_opt_ref_sorting(), which I think
> appends later keys to the end of the list, and compare_refs() which
> I think yields results when an earlier key in the last decides two
> are not equal and ignores the later keys.  The commit that added the
> description was c0f6dc9b ("git-for-each-ref.txt: minor
> improvements", 2008-06-05), and I think even back then the code in
> builtin-for-each-ref.c appended later keys at the end, and it seems
> nobody complained, so it is possible I am not reading the code right
> before fully adjusting to timezone change ;-)

Updated the docs.

>
> It is not too big a deal, but I find that liberal sprinkling of
> UNLEAK() is somewhat unsightly.  From the code we leave with this
> change, it is clear what we'd need to do when we make the code fully
> leak-proof (i.e. we'd have a several lines to free resources held by
> these two variables here, and then UNLEAK() that appear earlier in
> the function will become a "goto cleanup;" after setting appropriate
> value to "status"), so let's not worry about it.

Removed the "extra" unleak annotations within the if-return blocks,
but kept them at the end.

> Please do *NOT* step outside the pair of single quotes that protects
> the body of the test scriptlet and execute commands like "git
> rev-parse".  These execute in order to prepare the command line
> argument strings BEFORE test_expect_success can run (or the test
> framework decides if the test will be skipped via GIT_TEST_SKIP).
>
> Instead do it like so:
>
> cat >expect <<-EOF &&
> $(git rev-parse mark)   refs/tags/mark
> $(git rev-parse mark1.1)refs/tags/mark1.1
> $(git rev-parse mark1.2)refs/tags/mark1.2
> $(git rev-parse mark1.10)   refs/tags/mark1.10
> EOF
>
> I.e. the end token for << that is not quoted allows $var and $(cmd)
> to be substituted.
>
> Same comment applies throughout the remainder of this file.

Ah, thanks! I was looking for some way to allow the values to expand
within the proper test context.

So turns out '<<-\EOF' vs '<<-EOF' makes all the difference :)

> Other than that, this patch was a very pleasant read.  Thanks.

Thank you!

Re: Is support for 10.8 dropped?

[Eric Sunshine]

On Sun, Apr 8, 2018 at 7:55 PM, Igor Korot  wrote:
> On Sun, Apr 8, 2018, 6:23 PM Eric Sunshine  wrote:
>> And, as noted earlier, before running "make", you may need to create
>> config.mak to override some settings documented at the top of Makefile
>> (in particular, you may want to set NO_GETTEXT if you don't want to
>> install gettext and don't think you'll need it). As prerequisite,
>> you'll probably need to install OpenSSL.
>
> Is there a way to check for OpenSSL presence?

Not sure what you're asking. Are you asking how to determine if you
already have OpenSSL built on your machine?

Note that you might be able to get by without installing OpenSSL since
Git will try to use Apple's "Common Crypto" instead, so you could
define NO_OPENSSL in config.mak and see if the project builds.

[PATCH v13 2/4] ref-filter: make ref_array_item allocation more consistent

[Harald Nordgren]

From: Jeff King 

We have a helper function to allocate ref_array_item
structs, but it only takes a subset of the possible fields
in the struct as initializers. We could have it accept an
argument for _every_ field, but that becomes a pain for the
fields which some callers don't want to set initially.

Instead, let's be explicit that it takes only the minimum
required to create the ref, and that callers should then
fill in the rest themselves.

Signed-off-by: Jeff King 
Signed-off-by: Harald Nordgren 
---
 ref-filter.c | 18 +++---
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index ade97a848..c1c3cc948 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1824,15 +1824,18 @@ static const struct object_id *match_points_at(struct 
oid_array *points_at,
return NULL;
 }
 
-/* Allocate space for a new ref_array_item and copy the objectname and flag to 
it */
+/*
+ * Allocate space for a new ref_array_item and copy the name and oid to it.
+ *
+ * Callers can then fill in other struct members at their leisure.
+ */
 static struct ref_array_item *new_ref_array_item(const char *refname,
-const struct object_id *oid,
-int flag)
+const struct object_id *oid)
 {
struct ref_array_item *ref;
+
FLEX_ALLOC_STR(ref, refname, refname);
oidcpy(>objectname, oid);
-   ref->flag = flag;
 
return ref;
 }
@@ -1927,12 +1930,13 @@ static int ref_filter_handler(const char *refname, 
const struct object_id *oid,
 * to do its job and the resulting list may yet to be pruned
 * by maxcount logic.
 */
-   ref = new_ref_array_item(refname, oid, flag);
+   ref = new_ref_array_item(refname, oid);
ref->commit = commit;
+   ref->flag = flag;
+   ref->kind = kind;
 
REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
-   ref->kind = kind;
return 0;
 }
 
@@ -2169,7 +2173,7 @@ void pretty_print_ref(const char *name, const struct 
object_id *oid,
  const struct ref_format *format)
 {
struct ref_array_item *ref_item;
-   ref_item = new_ref_array_item(name, oid, 0);
+   ref_item = new_ref_array_item(name, oid);
ref_item->kind = ref_kind_from_refname(name);
show_ref_array_item(ref_item, format);
free_array_item(ref_item);
-- 
2.14.3 (Apple Git-98)

[PATCH v13 4/4] ls-remote: create '--sort' option

[Harald Nordgren]

Create a '--sort' option for ls-remote, based on the one from
for-each-ref. This e.g. allows ref names to be sorted by version
semantics, so that v1.2 is sorted before v1.10.

Signed-off-by: Harald Nordgren 
---

Notes:
Changes according to Junio Hamano's code review

 Documentation/git-ls-remote.txt | 17 -
 builtin/ls-remote.c | 25 +--
 t/t5512-ls-remote.sh| 54 +++--
 3 files changed, 86 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index 5f2628c8f..0e26a67a1 100644
--- a/Documentation/git-ls-remote.txt
+++ b/Documentation/git-ls-remote.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 
 [verse]
 'git ls-remote' [--heads] [--tags] [--refs] [--upload-pack=]
- [-q | --quiet] [--exit-code] [--get-url]
+ [-q | --quiet] [--exit-code] [--get-url] [--sort=]
  [--symref] [ [...]]
 
 DESCRIPTION
@@ -60,6 +60,17 @@ OPTIONS
upload-pack only shows the symref HEAD, so it will be the only
one shown by ls-remote.
 
+--sort=::
+   Sort based on the key given. Prefix `-` to sort in descending order
+   of the value. Supports "version:refname" or "v:refname" (tag names
+   are treated as versions). The "version:refname" sort order can also
+   be affected by the "versionsort.suffix" configuration variable.
+   See linkgit:git-for-each-ref[1] for more sort options, but be aware
+   that because `ls-remote` deals only with remotes, keys like
+   `committerdate` that requires access to the objects themselves will
+   not work for refs whose objects have not yet been fetched from the
+   remote.
+
 ::
The "remote" repository to query.  This parameter can be
either a URL or the name of a remote (see the GIT URLS and
@@ -90,6 +101,10 @@ EXAMPLES
c5db5456ae3b0873fc659c19fafdde22313cc441refs/tags/v0.99.2
7ceca275d047c90c0c7d5afb13ab97efdf51bd6erefs/tags/v0.99.3
 
+SEE ALSO
+
+linkgit:git-check-ref-format[1].
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 540d56429..b5ca67167 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -1,6 +1,7 @@
 #include "builtin.h"
 #include "cache.h"
 #include "transport.h"
+#include "ref-filter.h"
 #include "remote.h"
 
 static const char * const ls_remote_usage[] = {
@@ -43,10 +44,13 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
int show_symref_target = 0;
const char *uploadpack = NULL;
const char **pattern = NULL;
+   int i;
 
struct remote *remote;
struct transport *transport;
const struct ref *ref;
+   struct ref_array ref_array;
+   static struct ref_sorting *sorting = NULL, **sorting_tail = 
 
struct option options[] = {
OPT__QUIET(, N_("do not print remote URL")),
@@ -60,6 +64,8 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
OPT_BIT(0, "refs", , N_("do not show peeled tags"), 
REF_NORMAL),
OPT_BOOL(0, "get-url", _url,
 N_("take url..insteadOf into account")),
+   OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
+N_("field name to sort on"), 
_opt_ref_sorting),
OPT_SET_INT_F(0, "exit-code", ,
  N_("exit with exit code 2 if no matching refs are 
found"),
  2, PARSE_OPT_NOCOMPLETE),
@@ -68,6 +74,8 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
OPT_END()
};
 
+   memset(_array, 0, sizeof(ref_array));
+
argc = parse_options(argc, argv, prefix, options, ls_remote_usage,
 PARSE_OPT_STOP_AT_NON_OPTION);
dest = argv[0];
@@ -104,14 +112,27 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
if (!dest && !quiet)
fprintf(stderr, "From %s\n", *remote->url);
for ( ; ref; ref = ref->next) {
+   struct ref_array_item *item;
if (!check_ref_type(ref, flags))
continue;
if (!tail_match(pattern, ref->name))
continue;
+   item = ref_array_push(_array, ref->name, >old_oid);
+   item->symref = xstrdup_or_null(ref->symref);
+   }
+
+   if (sorting)
+   ref_array_sort(sorting, _array);
+
+   for (i = 0; i < ref_array.nr; i++) {
+   const struct ref_array_item *ref = ref_array.items[i];
if (show_symref_target && ref->symref)
-   printf("ref: %s\t%s\n", ref->symref, ref->name);
-   printf("%s\t%s\n", oid_to_hex(>old_oid), ref->name);
+   printf("ref: %s\t%s\n", ref->symref, 

[PATCH v13 3/4] ref-filter: factor ref_array pushing into its own function

[Harald Nordgren]

From: Jeff King 

In preparation for callers constructing their own ref_array
structs, let's move our own internal push operation into its
own function.

While we're at it, we can replace REALLOC_ARRAY() with
ALLOC_GROW(), which should give the growth operation
amortized linear complexity (as opposed to growing by one,
which is potentially quadratic, though in-place realloc
growth often makes this faster in practice).

Signed-off-by: Jeff King 
Signed-off-by: Harald Nordgren 
---
 ref-filter.c | 16 +---
 ref-filter.h |  8 
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index c1c3cc948..6e9328b27 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1840,6 +1840,18 @@ static struct ref_array_item *new_ref_array_item(const 
char *refname,
return ref;
 }
 
+struct ref_array_item *ref_array_push(struct ref_array *array,
+ const char *refname,
+ const struct object_id *oid)
+{
+   struct ref_array_item *ref = new_ref_array_item(refname, oid);
+
+   ALLOC_GROW(array->items, array->nr + 1, array->alloc);
+   array->items[array->nr++] = ref;
+
+   return ref;
+}
+
 static int ref_kind_from_refname(const char *refname)
 {
unsigned int i;
@@ -1930,13 +1942,11 @@ static int ref_filter_handler(const char *refname, 
const struct object_id *oid,
 * to do its job and the resulting list may yet to be pruned
 * by maxcount logic.
 */
-   ref = new_ref_array_item(refname, oid);
+   ref = ref_array_push(ref_cbdata->array, refname, oid);
ref->commit = commit;
ref->flag = flag;
ref->kind = kind;
 
-   REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
-   ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
return 0;
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index 68268f9eb..76cf87cb6 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -135,4 +135,12 @@ void setup_ref_filter_porcelain_msg(void);
 void pretty_print_ref(const char *name, const struct object_id *oid,
  const struct ref_format *format);
 
+/*
+ * Push a single ref onto the array; this can be used to construct your own
+ * ref_array without using filter_refs().
+ */
+struct ref_array_item *ref_array_push(struct ref_array *array,
+ const char *refname,
+ const struct object_id *oid);
+
 #endif /*  REF_FILTER_H  */
-- 
2.14.3 (Apple Git-98)

[PATCH v13 1/4] ref-filter: use "struct object_id" consistently

[Harald Nordgren]

From: Jeff King 

Internally we store a "struct object_id", and all of our
callers have one to pass us. But we insist that they peel it
to its bare-sha1 hash, which we then hashcpy() into place.
Let's pass it around as an object_id, which future-proofs us
for a post-sha1 world.

Signed-off-by: Jeff King 
Signed-off-by: Harald Nordgren 
---
 builtin/tag.c|  2 +-
 builtin/verify-tag.c |  2 +-
 ref-filter.c | 10 +-
 ref-filter.h |  2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index da186691e..42278f516 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -117,7 +117,7 @@ static int verify_tag(const char *name, const char *ref,
return -1;
 
if (format->format)
-   pretty_print_ref(name, oid->hash, format);
+   pretty_print_ref(name, oid, format);
 
return 0;
 }
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index ad7b79fa5..6fa04b751 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -72,7 +72,7 @@ int cmd_verify_tag(int argc, const char **argv, const char 
*prefix)
}
 
if (format.format)
-   pretty_print_ref(name, oid.hash, );
+   pretty_print_ref(name, , );
}
return had_error;
 }
diff --git a/ref-filter.c b/ref-filter.c
index 45fc56216..ade97a848 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1826,12 +1826,12 @@ static const struct object_id *match_points_at(struct 
oid_array *points_at,
 
 /* Allocate space for a new ref_array_item and copy the objectname and flag to 
it */
 static struct ref_array_item *new_ref_array_item(const char *refname,
-const unsigned char 
*objectname,
+const struct object_id *oid,
 int flag)
 {
struct ref_array_item *ref;
FLEX_ALLOC_STR(ref, refname, refname);
-   hashcpy(ref->objectname.hash, objectname);
+   oidcpy(>objectname, oid);
ref->flag = flag;
 
return ref;
@@ -1927,7 +1927,7 @@ static int ref_filter_handler(const char *refname, const 
struct object_id *oid,
 * to do its job and the resulting list may yet to be pruned
 * by maxcount logic.
 */
-   ref = new_ref_array_item(refname, oid->hash, flag);
+   ref = new_ref_array_item(refname, oid, flag);
ref->commit = commit;
 
REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
@@ -2165,11 +2165,11 @@ void show_ref_array_item(struct ref_array_item *info,
putchar('\n');
 }
 
-void pretty_print_ref(const char *name, const unsigned char *sha1,
+void pretty_print_ref(const char *name, const struct object_id *oid,
  const struct ref_format *format)
 {
struct ref_array_item *ref_item;
-   ref_item = new_ref_array_item(name, sha1, 0);
+   ref_item = new_ref_array_item(name, oid, 0);
ref_item->kind = ref_kind_from_refname(name);
show_ref_array_item(ref_item, format);
free_array_item(ref_item);
diff --git a/ref-filter.h b/ref-filter.h
index 0d98342b3..68268f9eb 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -132,7 +132,7 @@ void setup_ref_filter_porcelain_msg(void);
  * Print a single ref, outside of any ref-filter. Note that the
  * name must be a fully qualified refname.
  */
-void pretty_print_ref(const char *name, const unsigned char *sha1,
+void pretty_print_ref(const char *name, const struct object_id *oid,
  const struct ref_format *format);
 
 #endif /*  REF_FILTER_H  */
-- 
2.14.3 (Apple Git-98)

Re: Is support for 10.8 dropped?

[Eric Sunshine]

On Sun, Apr 8, 2018 at 6:37 PM, Igor Korot  wrote:
> MyMac:git-2.17.0 igorkorot$ make configure
> GIT_VERSION = 2.17.0
> GEN configure
> /bin/sh: autoconf: command not found
> make: *** [configure] Error 127
>
> Does this mean that something is a miss?

That means that you don't have the Autoconf tools installed, but don't
worry about it since use of the 'configure' script it very optional in
this project. Git tends to build just fine on most platforms without
running 'configure'. You should be able to get by just by typing
"make" in the git directory after downloading the source.

And, as noted earlier, before running "make", you may need to create
config.mak to override some settings documented at the top of Makefile
(in particular, you may want to set NO_GETTEXT if you don't want to
install gettext and don't think you'll need it). As prerequisite,
you'll probably need to install OpenSSL.

Re: [PATCH v2 0/4] Lazy-load trees when reading commit-graph

[Junio C Hamano]

Jeff King  writes:

> If I were doing it myself, I probably would have folded patches 1 and 3
> together. They are touching all the same spots, and it would be an error
> for any case converted in patch 1 to not get converted in patch 3. I'm
> assuming you caught them all due to Coccinelle, though IMHO it is
> somewhat overkill here. By folding them together the compiler could tell
> you which spots you missed.

Yeah, that approach would probably be a more sensible way to assure
the safety/correctness of the result to readers better.

>
> And going forward, I doubt it is going to be a common error for people
> to use maybe_tree directly. Between the name and the warning comment,
> you'd have to really try to shoot yourself in the foot with it. The
> primary concern was catching people using the existing "tree" name,
> whose semantics changed.

Yup.

Re: [PATCH 3/3] ref-filter: factor ref_array pushing into its own function

[Junio C Hamano]

Jeff King  writes:

> In preparation for callers constructing their own ref_array
> structs, let's move our own internal push operation into its
> own function.
>
> While we're at it, we can replace REALLOC_ARRAY() with
> ALLOC_GROW(), which should give the growth operation
> amortized linear complexity (as opposed to growing by one,
> which is potentially quadratic, though in-place realloc
> growth often makes this faster in practice).

Sorry, but I do not quite get the last part this paragraph.  Up to
but not including ", though in-place..." I would understand as:

 - With ALLOC_GROW(), we are explicit in that we are amortizing
   the allocation cost by growing in exponential batches.

 - With REALLOC_ARRAY(), we are telling the underlying
   realloc(3) that it is OK to pretend that we grow by one, but
   if that permission is literally followed, it could end up
   quadratic.

So if you have really a bad realloc(3) implementation, switching
to use ALLOC_GROW() is an improvement.

But after that I am not sure what you are getting at.  Do you mean
"In practice, a well-done realloc(3) does the amortizing internally
anyway, which makes it faster than the grow-by-one quadratic, so
this change may not make that much of a difference"?  Or do you mean
"In practice, a well-done realloc(3) internally amortizes better
than we do manually, so this change may hurt performance"?

In either case, I think this splitting into a helper is obviously a
good move, and using ALLOC_GROW() is conceptually cleaner, as we use
it everywhere and tend to avoid realloc(3) just to add one item.

Other than that small confusion (not even a nit), three patches were
pleasant read.  Thanks.

Re: [PATCH v4 1/3] builtin/config: introduce `--default`

[Junio C Hamano]

Jeff King  writes:

> On Wed, Apr 04, 2018 at 07:59:12PM -0700, Taylor Blau wrote:
>
>> @@ -286,6 +288,16 @@ static int get_value(const char *key_, const char 
>> *regex_)
>>  config_with_options(collect_config, ,
>>  _config_source, _options);
>>  
>> +if (!values.nr && default_value) {
>> +struct strbuf *item;
>> +ALLOC_GROW(values.items, values.nr + 1, values.alloc);
>> +item = [values.nr++];
>> +strbuf_init(item, 0);
>> +if (format_config(item, key_, default_value) < 0) {
>> +exit(1);
>> +}
>> +}
> ...
>
> It's obvious in this toy example, but that config call may be buried
> deep in a script. It'd probably be nicer for that exit(1) to be
> something like:
>
>   die(_("failed to format default config value"));

Together with key_ and default_value, you mean?

>
>> +test_expect_success 'does not allow --default without --get' '
>> +test_must_fail git config --default quux --unset a >output 2>&1 &&
>> +test_i18ngrep "\-\-default is only applicable to" output
>> +'
>
> I think "a" here needs to be "a.section". I get:
>
>   error: key does not contain a section: a

"section.var"?  That aside, even with a properly formatted key, I
seem to get an empty output here, so this may need a bit more work.

Re: [PATCH] specify encoding for sed command

[Junio C Hamano]

Stephon Harris  writes:

> Fixes issue with seeing `sed: RE error: illegal byte sequence` when running 
> git-completion.bash
> ---
>  contrib/completion/git-completion.bash | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index b09c8a23626b4..52a4ab5e2165a 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -282,7 +282,7 @@ __gitcomp ()
>  
>  # Clear the variables caching builtins' options when (re-)sourcing
>  # the completion script.
> -unset $(set |sed -ne 
> 's/^\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\)=.*/\1/p') 2>/dev/null
> +unset $(set |LANG=C sed -ne 
> 's/^\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\)=.*/\1/p') 2>/dev/null

Shouldn't LC_ALL and LANG both set and exported to C, as

 (1) ancient systems understand only LANG but not LC_*; and

 (2) modern ones that understand both give precedence to LC_ALL over
 LANG?

If we were to set only one, it is probably more sensible to set
LC_ALL, I suspect, but it may be better to set both, which sends a
sign to the readers that we know what we are doing ;-)

Re: [PATCH 1/9] git_config_set: fix off-by-two

[Junio C Hamano]

Johannes Schindelin  writes:

>> Yes, it is a workaround.  Making shell faster on windows would of
>> course be one possible solution to make t/t*.sh scripts go faster
>> ;-)  Or update parts of t/t*.sh so that the equivalent test coverage
>> can be kept while running making them go faster on Windows.
>
> What makes you think that I did not try my hardest for around 812 hours in
> total so far to make the shell faster?

Nowhere in these four lines I ever said that I think you did not
work hard to solve the performance issues you have.

Re: [PATCH v4] filter-branch: fix errors caused by refs that point at non-committish

[Junio C Hamano]

Yuki Kokubun  writes:

> References: <1521996898-7052-1-git-send-email-orga.chem@gmail.com>
> Content-Type: text/plain
>
> Sorry, I forgot add a line of "Reviewed-by".
> I'm gonna send the fixed patch again.

Do not worry too much about this.  Reviewed-by: added by patch
author is seen rather rarely, because the following sequence of
events need to happen:

 - The author sends a patch vN

 - A reviewer reviews vN and says "This is now Reviewed-by: me"

 - The author re-submits patch vN+1, where the only difference
   between vN and vN+1 is essentially _nothing_.  Except for the
   addition of "Reviewed-by:" by the reviewer.

But our typical patch injestion cycle is much faster and often the
last step does not happen ;-)

Re: After a rebase, ORIG_HEAD points to the previous tip of the branch?

[Junio C Hamano]

Ilya Kantor  writes:

> Immediately after a finished rebase, is it true that ORIG_HEAD points
> to the previous tip of the branch?
>
> So that `git reset --hard ORIG_HEAD` will cancel the rebase?

I wouldn't recommend people to depend on it; rather use "@{1}",
perhaps?

Before reflog was invented, ORIG_HEAD was primarily a handy way to
back out of "reset", but because the implementation of higher-level
porcelains like "rebase" may internally use things like "reset"
without giving it much thought about clobber ingORIG_HEAD (largely
because there is an expectation that modern end users would be using
reflog more than old-way ORIG_HEAD), cases like stopping in conflict
and getting told to skip and continue may not preserve ORIG_HEAD as
you wish it to.

Re: [RFC PATCH 1/1] completion: load completion file for external subcommand

[Junio C Hamano]

Florian Gamböck  writes:

> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index b09c8a236..e6114822c 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -3096,12 +3096,20 @@ __git_main ()
>   fi
>  

Sorry if I am asking something obvious, as I am not fluent in
bash-isms, but

>   local completion_func="_git_${command//-/_}"
> + if ! declare -f $completion_func >/dev/null 2>/dev/null; then
> + declare -f __load_completion >/dev/null 2>/dev/null &&
> + __load_completion "git-$command"

wouldn't the above be easier to read if it were

if ! declare ... $completion_func ... && declare -f __load_completion
then
__load_completion "git-$command"
fi

or is there a reason why it is better to &&-chain the check for
__load_completion with its use?  Same comment applies to the other
hunk.

Re: Is support for 10.8 dropped?

[Igor Korot]

Eric,
Sorry for multiple e-mails. I'm just trying to go thru with this.

This is what I tried:

[code]
MyMac:git-2.17.0 igorkorot$ make configure
GIT_VERSION = 2.17.0
GEN configure
/bin/sh: autoconf: command not found
make: *** [configure] Error 127
MyMac:git-2.17.0 igorkorot$
[/code]

Does this mean that something is a miss?

Thank you.


On Sun, Apr 8, 2018 at 5:10 PM, Igor Korot  wrote:
> Hi, Eric,
> First of, I already have Xcode installed along with the Developer Tools.
> Second, here is the list of the different dylib files I found on my system:
>
> MyMac:/ igorkorot$ find . -name libSystem.B.dylib
> ./Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS7.1.sdk/usr/lib/libSystem.B.dylib
> ./Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.8.sdk/usr/lib/libSystem.B.dylib
> ./Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.9.sdk/usr/lib/libSystem.B.dylib
> ./usr/lib/libSystem.B.dylib
>
> I'm hoping that the dylib in the MacOSX10.9 directory does have that
> symbol and maybe if I can check I can export this directory and let
> the linker pick it up.
>
> And apparently I was wrong:
>
> MyMac:/ igorkorot$ nm -gU
> /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.9.sdk/usr/lib/libSystem.B.dylib
> | grep strlcpy
> MyMac:/ igorkorot$ nm -gU
> /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.8.sdk/usr/lib/libSystem.B.dylib
> | grep strlcpy
> MyMac:/ igorkorot$
>
> I can try to build from source as this becomes my last resort. ;-)
>
> Thank you.
>
>
> On Sat, Apr 7, 2018 at 3:32 AM, Eric Sunshine  wrote:
>> On Fri, Apr 6, 2018 at 10:20 PM, Igor Korot  wrote:
> dyld: lazy symbol binding failed: Symbol not found: ___strlcpy_chk
>   Referenced from: /usr/local/git/libexec/git-core/git
>   Expected in: /usr/lib/libSystem.B.dylib

 It's not clear what installer you used? Was it the package from
 git-scm? Was it from Homebrew?
>>>
>>> I just tried the git-scm installer and got exactly the same error
>>> during the runtime.
>>
>> Have you tried any of the suggestions at these pages for resolving this 
>> issue?
>>
>> https://stackoverflow.com/questions/22920497/git-mountain-lion-dyld-lazy-symbol-binding-failed-symbol-not-found-str
>>
>> https://stackoverflow.com/questions/20929689/git-commands-not-working-in-mac-terminal-dyld-symbol-not-found-strlcpy-ch
>>
 I would guess that, even if the git-scm installer no longer supports
 10.8, it is likely that Homebrew does. Have you tried it?
>>>
>>> I don't want to pollute my system with Homebrew.
>>>
 If both those options fail, you can always build from source.
>>>
>>> Where do I get the soure code? And how do I build it?
>>> I guess I have only one option left. ;-)
>>
>> Source code for the latest release is at:
>> https://github.com/git/git/archive/v2.17.0.zip
>>
>> To build it, you'll need to have the MacOS Developer Tools installed.
>> It's also quite likely that you'll need to build some prerequisite
>> libraries; at minimum OpenSSL. You may be able to skip other libraries
>> if you don't care about the functionality. Build settings which you
>> may need to adjust to disable dependence on libraries in which you're
>> not interested are documented at the top of Makefile. Place overrides
>> for those settings in a file named config.mak in the git directory.

Re: [PATCH v12 4/4] ls-remote: create '--sort' option

[Junio C Hamano]

Harald Nordgren  writes:

> +--sort=::
> + Sort based on the key given. Prefix `-` to sort in descending order
> + of the value. Supports "version:refname" or "v:refname" (tag names
> + are treated as versions). The "version:refname" sort order can also
> + be affected by the "versionsort.suffix" configuration variable.
> + See linkgit:git-for-each-ref[1] for more sort options, but be aware
> + that because `ls-remote` deals only with remotes, any key like
> + `committerdate` that requires access to the object itself will
> + cause a failure.

I can connect "because it deals only with remotes" and "access to
the object may fail", but I wonder if we can clarify the former a
bit better to make it easier to understand for those who are not yet
familiar with Git.  

Keys like `committerdate` that require access to the object will
not work [***HOW??? Do we get a segfault or something???***] for
refs whose objects have not yet been fetched from the remote.

I added "for refs whose objects have not yet been fetched", whose
explicit-ness made "will not work" to be an OK expression, but
without it I would have suggested to rephrase it to "may not work".

This is a tangent but I suspect that the description of --sort in
git-for-each-ref documentation is wrong on the use of multiple
options, or I am misreading parse_opt_ref_sorting(), which I think
appends later keys to the end of the list, and compare_refs() which
I think yields results when an earlier key in the last decides two
are not equal and ignores the later keys.  The commit that added the
description was c0f6dc9b ("git-for-each-ref.txt: minor
improvements", 2008-06-05), and I think even back then the code in
builtin-for-each-ref.c appended later keys at the end, and it seems
nobody complained, so it is possible I am not reading the code right
before fully adjusting to timezone change ;-)

> + for (i = 0; i < ref_array.nr; i++) {
> + const struct ref_array_item *ref = ref_array.items[i];
>   if (show_symref_target && ref->symref)
> - printf("ref: %s\t%s\n", ref->symref, ref->name);
> - printf("%s\t%s\n", oid_to_hex(>old_oid), ref->name);
> + printf("ref: %s\t%s\n", ref->symref, ref->refname);
> + printf("%s\t%s\n", oid_to_hex(>objectname), ref->refname);
>   status = 0; /* we found something */
>   }
> +
> + UNLEAK(sorting);
> + UNLEAK(ref_array);
>   return status;
>  }

It is not too big a deal, but I find that liberal sprinkling of
UNLEAK() is somewhat unsightly.  From the code we leave with this
change, it is clear what we'd need to do when we make the code fully
leak-proof (i.e. we'd have a several lines to free resources held by
these two variables here, and then UNLEAK() that appear earlier in
the function will become a "goto cleanup;" after setting appropriate
value to "status"), so let's not worry about it.

> +test_expect_success 'ls-remote --sort="version:refname" --tags self' '
> + cat >expect <<-\EOF &&
> + '$(git rev-parse mark)' refs/tags/mark
> + '$(git rev-parse mark1.1)'  refs/tags/mark1.1
> + '$(git rev-parse mark1.2)'  refs/tags/mark1.2
> + '$(git rev-parse mark1.10)' refs/tags/mark1.10
> + EOF

Please do *NOT* step outside the pair of single quotes that protects
the body of the test scriptlet and execute commands like "git
rev-parse".  These execute in order to prepare the command line
argument strings BEFORE test_expect_success can run (or the test
framework decides if the test will be skipped via GIT_TEST_SKIP).

Instead do it like so:

cat >expect <<-EOF &&
$(git rev-parse mark)   refs/tags/mark
$(git rev-parse mark1.1)refs/tags/mark1.1
$(git rev-parse mark1.2)refs/tags/mark1.2
$(git rev-parse mark1.10)   refs/tags/mark1.10
EOF

I.e. the end token for << that is not quoted allows $var and $(cmd)
to be substituted.

Same comment applies throughout the remainder of this file.

Other than that, this patch was a very pleasant read.  Thanks.

Re: Is support for 10.8 dropped?

[Igor Korot]

Hi, Eric,
First of, I already have Xcode installed along with the Developer Tools.
Second, here is the list of the different dylib files I found on my system:

MyMac:/ igorkorot$ find . -name libSystem.B.dylib
./Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS7.1.sdk/usr/lib/libSystem.B.dylib
./Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.8.sdk/usr/lib/libSystem.B.dylib
./Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.9.sdk/usr/lib/libSystem.B.dylib
./usr/lib/libSystem.B.dylib

I'm hoping that the dylib in the MacOSX10.9 directory does have that
symbol and maybe if I can check I can export this directory and let
the linker pick it up.

And apparently I was wrong:

MyMac:/ igorkorot$ nm -gU
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.9.sdk/usr/lib/libSystem.B.dylib
| grep strlcpy
MyMac:/ igorkorot$ nm -gU
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.8.sdk/usr/lib/libSystem.B.dylib
| grep strlcpy
MyMac:/ igorkorot$

I can try to build from source as this becomes my last resort. ;-)

Thank you.


On Sat, Apr 7, 2018 at 3:32 AM, Eric Sunshine  wrote:
> On Fri, Apr 6, 2018 at 10:20 PM, Igor Korot  wrote:
 dyld: lazy symbol binding failed: Symbol not found: ___strlcpy_chk
   Referenced from: /usr/local/git/libexec/git-core/git
   Expected in: /usr/lib/libSystem.B.dylib
>>>
>>> It's not clear what installer you used? Was it the package from
>>> git-scm? Was it from Homebrew?
>>
>> I just tried the git-scm installer and got exactly the same error
>> during the runtime.
>
> Have you tried any of the suggestions at these pages for resolving this issue?
>
> https://stackoverflow.com/questions/22920497/git-mountain-lion-dyld-lazy-symbol-binding-failed-symbol-not-found-str
>
> https://stackoverflow.com/questions/20929689/git-commands-not-working-in-mac-terminal-dyld-symbol-not-found-strlcpy-ch
>
>>> I would guess that, even if the git-scm installer no longer supports
>>> 10.8, it is likely that Homebrew does. Have you tried it?
>>
>> I don't want to pollute my system with Homebrew.
>>
>>> If both those options fail, you can always build from source.
>>
>> Where do I get the soure code? And how do I build it?
>> I guess I have only one option left. ;-)
>
> Source code for the latest release is at:
> https://github.com/git/git/archive/v2.17.0.zip
>
> To build it, you'll need to have the MacOS Developer Tools installed.
> It's also quite likely that you'll need to build some prerequisite
> libraries; at minimum OpenSSL. You may be able to skip other libraries
> if you don't care about the functionality. Build settings which you
> may need to adjust to disable dependence on libraries in which you're
> not interested are documented at the top of Makefile. Place overrides
> for those settings in a file named config.mak in the git directory.

After a rebase, ORIG_HEAD points to the previous tip of the branch?

[Ilya Kantor]

Hi,

Immediately after a finished rebase, is it true that ORIG_HEAD points
to the previous tip of the branch?

So that `git reset --hard ORIG_HEAD` will cancel the rebase?

P.S. I'm asking because most of time that is so, but there is at least
one case when ORIG_HEAD breaks that assumption. Not sure if it's a bug
or not.

---
Best Regards,
Ilya Kantor

Re: reg. fatal: The remote end hung up unexpectedly on NFS

[Satya Prakash GS]

Avarab,

I enabled GIT_TRACE, packet, etc. I could not make much sense of the
output. I downloaded git source and started looking at the code.
Yeah, kernel compilation with failovers worked on our filesystem. We
tried xfs test suite with failovers, it worked. Since it's failing
with open source NFS, I am wondering if this even has anything to do
with filesystem.

Jeff,

Thank you for the clear instructions on strace.

Looks like 5 and 6 are two ends of a pipe.

read(5, "\27\3\3\5r", 5)  = 5
read(5, 
"S\10n\230i\257\27\2\333&\335jou~\3036}S\273\212\250\33\310\216b\253\3505\332\266X"...,
1394) = 1394
write(6, 
"\2530\253x\27\247iz\246\321*\v\33\306.\221I\36\4\304D\\\360\361\306I\213\304\245m\247\30"...,
1370) = 1370
<... read resumed>
"\2530\253x\27\247iz\246\321*\v\33\306.\221I\36\4\304D\\\360\361\306I\213\304\245m\247\30"...,
5071) = 1370
rt_sigaction(SIGPIPE, {SIG_IGN, [PIPE], SA_RESTORER|SA_RESTART,
0x7f038e8bb890},  
read(0,  
<... rt_sigaction resumed> NULL, 8) = 0
poll([{fd=5, events=POLLIN}], 1, 1000) = 1 ([{fd=5, revents=POLLIN}])
rt_sigaction(SIGPIPE, NULL, {SIG_IGN, [PIPE], SA_RESTORER|SA_RESTART,
0x7f038e8bb890}, 8) = 0
rt_sigaction(SIGPIPE, {SIG_IGN, [PIPE], SA_RESTORER|SA_RESTART,
0x7f038e8bb890}, NULL, 8) = 0
poll([{fd=5, events=POLLIN|POLLPRI|POLLRDNORM|POLLRDBAND}], 1, 0) = 1
([{fd=5, revents=POLLIN|POLLRDNORM}])
read(5, "\27\3\3\5r", 5)  = 5
read(5, 
"S\10n\230i\257\27\3]T\0{\232\253\377$\265\277\211o.\314T\315\257\335j\322\367\31o\262"...,
1394) = 1394

> Write preceding the last read has succeeded
write(6, 
"\237%\204W$\236\177\305@\210+\236\227.\316\226\250\t\256:\27?};\270^A\317\204\222\35]"...,
1370) = 1370


<... read resumed>
"\237%\204W$\236\177\305@\210+\236\227.\316\226\250\t\256:\27?};\270^A\317\204\222\35]"...,
3701) = 1370
rt_sigaction(SIGPIPE, {SIG_IGN, [PIPE], SA_RESTORER|SA_RESTART,
0x7f038e8bb890},  
read(0,  
<... rt_sigaction resumed> NULL, 8) = 0
poll([{fd=5, events=POLLIN}], 1, 1000) = 1 ([{fd=5, revents=POLLIN}])
rt_sigaction(SIGPIPE, NULL, {SIG_IGN, [PIPE], SA_RESTORER|SA_RESTART,
0x7f038e8bb890}, 8) = 0
rt_sigaction(SIGPIPE, {SIG_IGN, [PIPE], SA_RESTORER|SA_RESTART,
0x7f038e8bb890}, NULL, 8) = 0
poll([{fd=5, events=POLLIN|POLLPRI|POLLRDNORM|POLLRDBAND}], 1, 0) = 1
([{fd=5, revents=POLLIN|POLLRDNORM}])
read(5, "\27\3\3\5r", 5)  = 5

> Problematic reads
read(5, 
"S\10n\230i\257\27\4\220\243\375\3772\213?\267\356V\246r\226`\223\253^\310\314\207\222\22%\376"...,
1394) = 363
read(5, "", 1031) = 0
>>>

write(5, "\25\3\3\0\32\265(Nk5\330Y\4!\374S\204\377\304\0166j\rV\305;e3\347",
31) = 31
close(5)

rt_sigaction(SIGPIPE, {SIG_IGN, [PIPE], SA_RESTORER|SA_RESTART,
0x7f038e8bb890}, NULL, 8) = 0
rt_sigaction(SIGPIPE, {0x441ed0, [PIPE], SA_RESTORER|SA_RESTART,
0x7f038e8bb890}, NULL, 8) = 0
write(2, "error: RPC failed; result=18, HT"..., 46) = 46
brk(0x1597000)= 0x1597000
close(6 
<... read resumed> "", 2331)  = 0
<... close resumed> ) = 0
read(7, "", 4096) = 4
write(2, "fatal: The remote end hung up un"..., 43 
read(7,  
<... write resumed> ) = 43
close(4 
<... read resumed> "", 4096)  = 0
<... close resumed> )

Last read from pipe (fd 5) was a partial read. Write has succeeded on
the pipe but read failed, that's strange. In this case, it doesn't
even look like the write was interleaved.

Thanks,
Satya.

On Sat, Apr 7, 2018 at 1:18 AM, Jeff King  wrote:
> On Fri, Apr 06, 2018 at 11:55:51PM +0530, Satya Prakash GS wrote:
>
>> We have a distributed filesystem with NFS access. On the NFS mount, I
>> was doing a git-clone and if NFS server crashed and came back up while
>> the clone is going on, clone fails with the below message:
>>
>> git clone https://sa...@github.com/fs/private-qa.git
>>
>> remote: Counting objects: 139419, done.
>> remote: Compressing objects: 100% (504/504), done.
>> Receiving objects:   7% (9760/139419), 5.32 MiB | 5.27 MiB/s
>> error: RPC failed; result=18, HTTP code = 200 MiB | 96.00 KiB/s
>> fatal: The remote end hung up unexpectedly
>> fatal: early EOF
>> fatal: index-pack failed
>
> Curl's result=18 is CURLE_PARTIAL_FILE. Usually that means the other
> side hung up partway through. But given the NFS symptoms you describe, I
> wonder if fwrite() to the file simply returned an error, and curl gave
> up.
>
>> On NFS server crash, it usually takes a minute or two for our
>> filesystem to failover to new NFS server. Initially I suspected it had
>> something to do with the filesystem, like attributes of the file
>> written by git weren't matching what it was expecting but the same
>> test fails on open source NFS server. While clone is going on, if I
>> stopped the open source NFS server for 2 minutes and restarted it, git
>> clone fails.
>>
>> Another interesting thing is, if the restart happens within a few
>> seconds, git clone succeeds.
>>
>> Sideband_demux fails while trying to read from the pipe. 

Re: [RFC PATCH 1/1] completion: Load completion file for external subcommand

[Florian Gamböck]

Ah, sorry, please ignore this one.

[RFC PATCH 0/1] completion: Dynamic completion loading

[Florian Gamböck]

In this small patch I want to introduce a way to dynamically load completion 
scripts for external subcommands.

A few years ago, you would put a completion script (which defines a Bash 
function _git_foo for a custom git subcommand foo) into 
/etc/bash_completion.d, or save it somewhere in your $HOME and source it 
manually in your .bashrc.

Starting with bash-completion v2.0 (or, to be absolutely exact, the preview 
versions started at v1.90), completion scripts are not sourced at bash startup 
anymore. Rather, when typing in a command and trigger completion by pressing 
the TAB key, the new bash-completion's main script looks for a script with the 
same name as the typed command in the completion directory, sources it, and 
provides the completions defined in this script.

However, that only works for top level commands. After a completion script has 
been found, the logic is delegated to this script. This means, that the 
completion of subcommands must be handled by the corresponding completion 
script.

As it is now, git is perfectly able to call custom completion functions, iff 
they are already defined when calling the git completion. With my patch, git 
uses an already defined loader function of bash-completion (or continues 
silently if this function is not found), loads an external completion script, 
and provides its completions.

An example for an external completion script would be 
/usr/share/bash-completion/completions/git-foo for a git subcommand foo.

Florian Gamböck (1):
  completion: Load completion file for external subcommand

 contrib/completion/git-completion.bash | 8 
 1 file changed, 8 insertions(+)

-- 
2.16.1

[RFC PATCH 1/1] completion: Load completion file for external subcommand

[Florian Gamböck]

Adding external subcommands to Git is as easy as to put an executable
file git-foo into PATH. Packaging such subcommands for a Linux
distribution can be achieved by unpacking the executable into /usr/bin
of the user's system. Adding system-wide completion scripts for new
subcommands, however, can be a bit tricky.

Since bash-completion started to use dynamical loading of completion
scripts somewhere around v2.0, it is no longer sufficient to drop a
completion script of a subcommand into the standard completions path,
/usr/share/bash-completion/completions, since this script will not be
loaded if called as a git subcommand.

For example, look at https://bugs.gentoo.org/544722. To give a short
summary: The popular git-flow subcommand provides a completion script,
which gets installed as /usr/share/bash-completion/completions/git-flow.

If you now type into a Bash shell:

git flow 

You will not get any completions, because bash-completion only loads
completions for git and git has no idea that git-flow is defined in
another file. You have to load this script manually or trigger the
dynamic loader with:

git-flow  # Please notice the dash instead of whitespace

This will not complete anything either, because it only defines a Bash
function, without generating completions. But now the correct completion
script has been loaded and the first command can use the completions.

So, the goal is now to teach the git completion script to consider the
possibility of external completion scripts for subcommands, but of
course without breaking current workflows.

I think the easiest method is to use a function that is defined by
bash-completion v2.0+, namely __load_completion. It will take care of
loading the correct script if present. Afterwards, the git completion
script behaves as usual.

This way we can leverage bash-completion's dynamic loading for git
subcommands and make it easier for developers to distribute custom
completion scripts.

Signed-off-by: Florian Gamböck 
---
 contrib/completion/git-completion.bash | 8 
 1 file changed, 8 insertions(+)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index b09c8a236..e6114822c 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3096,12 +3096,20 @@ __git_main ()
fi
 
local completion_func="_git_${command//-/_}"
+   if ! declare -f $completion_func >/dev/null 2>/dev/null; then
+   declare -f __load_completion >/dev/null 2>/dev/null &&
+   __load_completion "git-$command"
+   fi
declare -f $completion_func >/dev/null 2>/dev/null && $completion_func 
&& return
 
local expansion=$(__git_aliased_command "$command")
if [ -n "$expansion" ]; then
words[1]=$expansion
completion_func="_git_${expansion//-/_}"
+   if ! declare -f $completion_func >/dev/null 2>/dev/null; then
+   declare -f __load_completion >/dev/null 2>/dev/null &&
+   __load_completion "git-$expansion"
+   fi
declare -f $completion_func >/dev/null 2>/dev/null && 
$completion_func
fi
 }
-- 
2.16.1

[RFC PATCH 1/1] completion: load completion file for external subcommand

[Florian Gamböck]

Adding external subcommands to Git is as easy as to put an executable
file git-foo into PATH. Packaging such subcommands for a Linux
distribution can be achieved by unpacking the executable into /usr/bin
of the user's system. Adding system-wide completion scripts for new
subcommands, however, can be a bit tricky.

Since bash-completion started to use dynamical loading of completion
scripts somewhere around v2.0, it is no longer sufficient to drop a
completion script of a subcommand into the standard completions path,
/usr/share/bash-completion/completions, since this script will not be
loaded if called as a git subcommand.

For example, look at https://bugs.gentoo.org/544722. To give a short
summary: The popular git-flow subcommand provides a completion script,
which gets installed as /usr/share/bash-completion/completions/git-flow.

If you now type into a Bash shell:

git flow 

You will not get any completions, because bash-completion only loads
completions for git and git has no idea that git-flow is defined in
another file. You have to load this script manually or trigger the
dynamic loader with:

git-flow  # Please notice the dash instead of whitespace

This will not complete anything either, because it only defines a Bash
function, without generating completions. But now the correct completion
script has been loaded and the first command can use the completions.

So, the goal is now to teach the git completion script to consider the
possibility of external completion scripts for subcommands, but of
course without breaking current workflows.

I think the easiest method is to use a function that is defined by
bash-completion v2.0+, namely __load_completion. It will take care of
loading the correct script if present. Afterwards, the git completion
script behaves as usual.

This way we can leverage bash-completion's dynamic loading for git
subcommands and make it easier for developers to distribute custom
completion scripts.

Signed-off-by: Florian Gamböck 
---
 contrib/completion/git-completion.bash | 8 
 1 file changed, 8 insertions(+)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index b09c8a236..e6114822c 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3096,12 +3096,20 @@ __git_main ()
fi
 
local completion_func="_git_${command//-/_}"
+   if ! declare -f $completion_func >/dev/null 2>/dev/null; then
+   declare -f __load_completion >/dev/null 2>/dev/null &&
+   __load_completion "git-$command"
+   fi
declare -f $completion_func >/dev/null 2>/dev/null && $completion_func 
&& return
 
local expansion=$(__git_aliased_command "$command")
if [ -n "$expansion" ]; then
words[1]=$expansion
completion_func="_git_${expansion//-/_}"
+   if ! declare -f $completion_func >/dev/null 2>/dev/null; then
+   declare -f __load_completion >/dev/null 2>/dev/null &&
+   __load_completion "git-$expansion"
+   fi
declare -f $completion_func >/dev/null 2>/dev/null && 
$completion_func
fi
 }
-- 
2.16.1

Re: [PATCH v6 6/6] worktree: teach "add" to check out existing branches

[Thomas Gummerer]

On 04/08, Eric Sunshine wrote:
> On Sat, Mar 31, 2018 at 11:18 AM, Thomas Gummerer  
> wrote:
> > [...]
> > However we can do a little better than that, and check the branch out if
> > it is not checked out anywhere else.  This will help users who just want
> > to check an existing branch out into a new worktree, and save a few
> > keystrokes.
> > [...]
> > Signed-off-by: Thomas Gummerer 
> > ---
> > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> > @@ -61,8 +61,13 @@ $ git worktree add --track -b   
> > /
> >  If `` is omitted and neither `-b` nor `-B` nor `--detach` used,
> > +then, as a convenience, a worktree with a branch named after
> > +`$(basename )` (call it ``) is created.
> 
> I had a hard time digesting this. I _think_ it wants to say:
> 
> If `` is omitted and neither `-b` nor `-B` nor
> `--detach` is used, then, as a convenience, the new worktree is
> associated with a branch (call it ``) named after
> `$(basename )`.

Yeah, this is what it wants to say, and what you have sounds much
nicer, will change.

> >  If ``
> > +doesn't exist, a new branch based on HEAD is automatically created as
> > +if `-b ` was given.  If `` exists in the repository,
> 
> Maybe: s/exists in the repository/does exist/
> Or: s/.../is a local branch/
> 
> Though, the latter may be getting too pedantic.
> 
> > +it will be checked out in the new worktree, if it's not checked out
> > +anywhere else, otherwise the command will refuse to create the
> > +worktree (unless `--force` is used).
> > diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> > @@ -198,13 +198,24 @@ test_expect_success '"add" with  omitted' '
> > +test_expect_success '"add" checks out existing branch of dwimd name' '
> > +   git branch dwim HEAD~1 &&
> > +   git worktree add dwim &&
> > +   test_cmp_rev HEAD~1 dwim &&
> > +   (
> > +   cd dwim &&
> > +   test_cmp_rev dwim HEAD
> 
> Nit: Argument order of the two test_cmp_rev() invocations differs.
> 
> > +   )
> > +'
> > +
> > +test_expect_success '"add " dwim fails with checked out branch' '
> > +   git checkout -b test-branch &&
> > +   test_must_fail git worktree add test-branch &&
> > +   test_path_is_missing test-branch
> > +'
> > +
> > +test_expect_success '"add --force" with existing dwimd name doesnt die' '
> > +   git worktree add --force test-branch
> >  '
> 
> Maybe make this last test slightly more robust by having it "git
> checkout test-branch" before "git worktree add ..." to protect against
> someone inserting a new test (which checks out some other branch)
> between these two. Probably not worth a re-roll.

As I'll have to re-roll anyway for the other suggestions, I'll change
this as well.

Thanks for your review!

Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches

[Thomas Gummerer]

On 04/08, Eric Sunshine wrote:
> On Sat, Mar 31, 2018 at 11:17 AM, Thomas Gummerer  
> wrote:
> > This round should fix all the UI issues Eric found in the last round.
> > The changes I made in a bit more detail:
> >
> > - added a new commit introducing a new hidden --show-new-head-line
> >   flag in 'git reset'.  This is used to suppress the "HEAD is now at
> >   ..."  line that 'git reset --hard' usually prints, so we can replace
> >   it with our own "New worktree HEAD is now at ..." line instead,
> >   while keeping the progress indicator for larger repositories.
> 
> As with Junio, I'm fine with this hidden option (for now), however, I
> think you can take this a step further. Rather than having a (hidden)
> git-reset option which suppresses "HEAD is now at...", instead have a
> (hidden) option which augments the message. For example,
> --new-head-desc="New worktree" would make it output "New worktree HEAD
> is now at...". Changes to builtin/reset.c to support this would hardly
> be larger than the changes you already made.
> 
> The major benefit is that patch 3/6 no longer has to duplicate the
> code from builtin/reset.c:print_new_head_line() just to print its own
> "New worktree HEAD is now at..." message. (As for the argument that
> "git worktree add" must duplicate that code because it wants the
> message on stderr, whereas git-reset prints it to stdout, I don't see
> why git-worktree puts those messages to stderr in the first place. As
> far as I can tell, it would be equally valid to print them to stdout.)

I didn't think of that, but I think that's nicer indeed.  Will change.
This will also be nicer when we're in a position to remove the hidden
option and do all of this with internal functions.  Then we can just
re-use the new function that also takes a prefix at that point (after
moving the function to 'libgit.a' of course.

> > Some examples of the new UI behaviour here for reference:
> >
> >  - guess-remote mode
> >
> > $ git worktree add --guess-remote ../next
> > Creating branch 'next'
> > Branch 'next' set up to track remote branch 'next' from 'origin'.
> > New worktree HEAD is now at caa68db14 Merge branch 
> > 'sb/packfiles-in-repository' into next
> >
> >  - original dwim (create a branch based on the current HEAD)
> >
> > $ git worktree add ../test
> > Creating branch 'test'
> > New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'
> >
> >  - new dwim (check out existing branch)
> >
> > $ git worktree add ../test
> > Checking out branch 'test'
> > New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'
> >
> >  - no new branch created
> >
> > $ git worktree add ../test2 origin/master
> > New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'
> 
> I like the "creating" or "checking out" messages we now get for all
> the DWIM cases. I wonder if it would make sense to print "Checkout out
> blah..." for this case too. It's certainly not necessary since the
> user specified  explicitly, but it would make the UI even
> more consistent, and address your subsequent comment about missing
> context above the "Checking out files: ...%" line for this case.
> Thoughts?

Let me think through some of the cases here, of 'git worktre add
 ' with various flags and what the UI would be with
that added:

  - no flags:

$ git worktree add ../test origin/master
Checking out 'origin/master'
Checking out files: ...%
New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'

  - -b branch:

$ git worktree add -b test ../test origin/master
Creating branch 'test'
Checking out 'origin/master'
Checking out files: ...%
New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'

Would we want to omit the "Checking out ..." here?  I'm leaning
towards yes, but dunno?

  - --no-checkout

$ git worktree add --no-checkout test ../test origin/master
New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'

  - Original dwim with --detach flag

$ git worktree add --detach ../test
Checking out 'c2a499e6c'
Checking out files: ...%
New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'

Looking at this, I'm not sure what's best here.  I'm not sure I'm a
fan of the duplicate "Checking out " message (I assume that's what you
meant above, or did you mean just "Checkout ..."?)

I als don't think it gives too much context compared to just "Checking
out files: ...%".  I think it gives a bit more context when that
message is not displayed at all, as it shows whether files are checked
out or not, but if we do that, when we create a new branch, the amount
of output we'd display is getting a bit long, to the point where I
suspect users would just not read it anymore.

So I personally don't feel like this is worth it, even though it may
give some context in some cases.  But I'm also far from an expert in

Re: Is offloading to GPU a worthwhile feature?

[Jakub Narebski]

Hello,

Konstantin Ryabitsev  writes:

> This is an entirely idle pondering kind of question, but I wanted to
> ask. I recently discovered that some edge providers are starting to
> offer systems with GPU cards in them -- primarily for clients that need
> to provide streaming video content, I guess. As someone who needs to run
> a distributed network of edge nodes for a fairly popular git server, I
> wondered if git could at all benefit from utilizing a GPU card for
> something like delta calculations or compression offload, or if benefits
> would be negligible.
>
> I realize this would be silly amounts of work. But, if it's worth it,
> perhaps we can benefit from all the GPU computation libs written for
> cryptocoin mining and use them for something good. :)

The problem is that you need to transfer the data from the main memory
(host memory) geared towards low-latency thanks to cache hierarchy, to
the GPU memory (device memory) geared towards bandwidth and parallel
access, and back again.  So to make sense the time for copying data plus
the time to perform calculations on GPU (and not all kinds of
computations can be speed up on GPU -- you need fine-grained massively
data-parallel task) must be less than time to perform calculations on
CPU (with multi-threading).

Also you would need to keep non-GPU and GPGPU code in sync.  Some parts
of code do not change much; and there also solutions to generate dual
code from one source.

Still, it might be good idea,
-- 
Jakub Narębski

Re: [PATCH v7 13/14] commit-graph: build graph from starting commits

[Jakub Narebski]

Derrick Stolee  writes:

> @@ -96,10 +101,12 @@ static int graph_write(int argc, const char **argv)
>builtin_commit_graph_write_options,
>builtin_commit_graph_write_usage, 0);
>  
> + if (opts.stdin_packs && opts.stdin_commits)
> + die(_("cannot use both --stdin-commits and --stdin-packs"));

Here error message is marked for translation, which is not the case for
other patches in the series.

-- 
Jakub Narębski

Re: [PATCH v7 09/14] commit-graph: add core.commitGraph setting

[Jakub Narebski]

Derrick Stolee  writes:

> From: Derrick Stolee 
>
> The commit graph feature is controlled by the new core.commitGraph config
> setting. This defaults to 0, so the feature is opt-in.

Nice.  That's how bitmaps feature was introduced, I think.  I guess that
in the future reading would be opt-out, isn't it, same as currently for
bitmaps (pack.useBitmaps setting).

>
> The intention of core.commitGraph is that a user can always stop checking
> for or parsing commit graph files if core.commitGraph=0.

Shouldn't it be "false", not "0"?

> Signed-off-by: Derrick Stolee 
> ---
>  Documentation/config.txt | 4 
>  cache.h  | 1 +
>  config.c | 5 +
>  environment.c| 1 +
>  4 files changed, 11 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 4e0cff87f6..e5c7013fb0 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -898,6 +898,10 @@ core.notesRef::
>  This setting defaults to "refs/notes/commits", and it can be overridden by
>  the `GIT_NOTES_REF` environment variable.  See linkgit:git-notes[1].
>  
> +core.commitGraph::
> + Enable git commit graph feature. Allows reading from the
> + commit-graph file.
> +

Good.  It would be nice to have "Defaults to false." and possibly also
reference to "git-commit-graph(1)" manpage for more details, though.

>  core.sparseCheckout::
>   Enable "sparse checkout" feature. See section "Sparse checkout" in
>   linkgit:git-read-tree[1] for more information.
[...]

-- 
Jakub Narębski

Re: [PATCH v7 08/14] commit-graph: implement git commit-graph read

[Jakub Narebski]

Derrick Stolee  writes:

[...]
>  EXAMPLES
>  
> @@ -45,6 +51,12 @@ EXAMPLES
>  $ git commit-graph write
>  
>  
> +* Read basic information from the commit-graph file.
> ++
> +
> +$ git commit-graph read
> +

It would be better to have example output of this command here, perhaps
together with ASCII-art diagram of the code graph.

[...]
> + if (!graph)
> + die("graph file %s does not exist", graph_name);
> + FREE_AND_NULL(graph_name);

Shouldn't the error message be marked up for translation, too?

Regards,
-- 
Jakub Narębski

[PATCH v12 4/4] ls-remote: create '--sort' option

[Harald Nordgren]

Create a '--sort' option for ls-remote, based on the one from
for-each-ref. This e.g. allows ref names to be sorted by version
semantics, so that v1.2 is sorted before v1.10.

Signed-off-by: Harald Nordgren 
---

Notes:
Changes according to Eric Sunshine's code review

 Documentation/git-ls-remote.txt | 16 -
 builtin/ls-remote.c | 30 +---
 t/t5512-ls-remote.sh| 52 -
 3 files changed, 88 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index 5f2628c8f..80a09b518 100644
--- a/Documentation/git-ls-remote.txt
+++ b/Documentation/git-ls-remote.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 
 [verse]
 'git ls-remote' [--heads] [--tags] [--refs] [--upload-pack=]
- [-q | --quiet] [--exit-code] [--get-url]
+ [-q | --quiet] [--exit-code] [--get-url] [--sort=]
  [--symref] [ [...]]
 
 DESCRIPTION
@@ -60,6 +60,16 @@ OPTIONS
upload-pack only shows the symref HEAD, so it will be the only
one shown by ls-remote.
 
+--sort=::
+   Sort based on the key given. Prefix `-` to sort in descending order
+   of the value. Supports "version:refname" or "v:refname" (tag names
+   are treated as versions). The "version:refname" sort order can also
+   be affected by the "versionsort.suffix" configuration variable.
+   See linkgit:git-for-each-ref[1] for more sort options, but be aware
+   that because `ls-remote` deals only with remotes, any key like
+   `committerdate` that requires access to the object itself will
+   cause a failure.
+
 ::
The "remote" repository to query.  This parameter can be
either a URL or the name of a remote (see the GIT URLS and
@@ -90,6 +100,10 @@ EXAMPLES
c5db5456ae3b0873fc659c19fafdde22313cc441refs/tags/v0.99.2
7ceca275d047c90c0c7d5afb13ab97efdf51bd6erefs/tags/v0.99.3
 
+SEE ALSO
+
+linkgit:git-check-ref-format[1].
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 540d56429..d3851074c 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -1,6 +1,7 @@
 #include "builtin.h"
 #include "cache.h"
 #include "transport.h"
+#include "ref-filter.h"
 #include "remote.h"
 
 static const char * const ls_remote_usage[] = {
@@ -43,10 +44,13 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
int show_symref_target = 0;
const char *uploadpack = NULL;
const char **pattern = NULL;
+   int i;
 
struct remote *remote;
struct transport *transport;
const struct ref *ref;
+   struct ref_array ref_array;
+   static struct ref_sorting *sorting = NULL, **sorting_tail = 
 
struct option options[] = {
OPT__QUIET(, N_("do not print remote URL")),
@@ -60,6 +64,8 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
OPT_BIT(0, "refs", , N_("do not show peeled tags"), 
REF_NORMAL),
OPT_BOOL(0, "get-url", _url,
 N_("take url..insteadOf into account")),
+   OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
+N_("field name to sort on"), 
_opt_ref_sorting),
OPT_SET_INT_F(0, "exit-code", ,
  N_("exit with exit code 2 if no matching refs are 
found"),
  2, PARSE_OPT_NOCOMPLETE),
@@ -68,6 +74,8 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
OPT_END()
};
 
+   memset(_array, 0, sizeof(ref_array));
+
argc = parse_options(argc, argv, prefix, options, ls_remote_usage,
 PARSE_OPT_STOP_AT_NON_OPTION);
dest = argv[0];
@@ -90,6 +98,7 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
 
if (get_url) {
printf("%s\n", *remote->url);
+   UNLEAK(sorting);
return 0;
}
 
@@ -98,20 +107,35 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
transport_set_option(transport, TRANS_OPT_UPLOADPACK, 
uploadpack);
 
ref = transport_get_remote_refs(transport);
-   if (transport_disconnect(transport))
+   if (transport_disconnect(transport)) {
+   UNLEAK(sorting);
return 1;
+   }
 
if (!dest && !quiet)
fprintf(stderr, "From %s\n", *remote->url);
for ( ; ref; ref = ref->next) {
+   struct ref_array_item *item;
if (!check_ref_type(ref, flags))
continue;
if (!tail_match(pattern, ref->name))
continue;
+   item = ref_array_push(_array, ref->name, >old_oid);
+   item->symref = xstrdup_or_null(ref->symref);
+  

[PATCH v12 2/4] ref-filter: make ref_array_item allocation more consistent

[Harald Nordgren]

From: Jeff King 

We have a helper function to allocate ref_array_item
structs, but it only takes a subset of the possible fields
in the struct as initializers. We could have it accept an
argument for _every_ field, but that becomes a pain for the
fields which some callers don't want to set initially.

Instead, let's be explicit that it takes only the minimum
required to create the ref, and that callers should then
fill in the rest themselves.

Signed-off-by: Jeff King 
Signed-off-by: Harald Nordgren 
---
 ref-filter.c | 18 +++---
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index ade97a848..c1c3cc948 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1824,15 +1824,18 @@ static const struct object_id *match_points_at(struct 
oid_array *points_at,
return NULL;
 }
 
-/* Allocate space for a new ref_array_item and copy the objectname and flag to 
it */
+/*
+ * Allocate space for a new ref_array_item and copy the name and oid to it.
+ *
+ * Callers can then fill in other struct members at their leisure.
+ */
 static struct ref_array_item *new_ref_array_item(const char *refname,
-const struct object_id *oid,
-int flag)
+const struct object_id *oid)
 {
struct ref_array_item *ref;
+
FLEX_ALLOC_STR(ref, refname, refname);
oidcpy(>objectname, oid);
-   ref->flag = flag;
 
return ref;
 }
@@ -1927,12 +1930,13 @@ static int ref_filter_handler(const char *refname, 
const struct object_id *oid,
 * to do its job and the resulting list may yet to be pruned
 * by maxcount logic.
 */
-   ref = new_ref_array_item(refname, oid, flag);
+   ref = new_ref_array_item(refname, oid);
ref->commit = commit;
+   ref->flag = flag;
+   ref->kind = kind;
 
REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
-   ref->kind = kind;
return 0;
 }
 
@@ -2169,7 +2173,7 @@ void pretty_print_ref(const char *name, const struct 
object_id *oid,
  const struct ref_format *format)
 {
struct ref_array_item *ref_item;
-   ref_item = new_ref_array_item(name, oid, 0);
+   ref_item = new_ref_array_item(name, oid);
ref_item->kind = ref_kind_from_refname(name);
show_ref_array_item(ref_item, format);
free_array_item(ref_item);
-- 
2.14.3 (Apple Git-98)

[PATCH v12 3/4] ref-filter: factor ref_array pushing into its own function

[Harald Nordgren]

From: Jeff King 

In preparation for callers constructing their own ref_array
structs, let's move our own internal push operation into its
own function.

While we're at it, we can replace REALLOC_ARRAY() with
ALLOC_GROW(), which should give the growth operation
amortized linear complexity (as opposed to growing by one,
which is potentially quadratic, though in-place realloc
growth often makes this faster in practice).

Signed-off-by: Jeff King 
Signed-off-by: Harald Nordgren 
---
 ref-filter.c | 16 +---
 ref-filter.h |  8 
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index c1c3cc948..6e9328b27 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1840,6 +1840,18 @@ static struct ref_array_item *new_ref_array_item(const 
char *refname,
return ref;
 }
 
+struct ref_array_item *ref_array_push(struct ref_array *array,
+ const char *refname,
+ const struct object_id *oid)
+{
+   struct ref_array_item *ref = new_ref_array_item(refname, oid);
+
+   ALLOC_GROW(array->items, array->nr + 1, array->alloc);
+   array->items[array->nr++] = ref;
+
+   return ref;
+}
+
 static int ref_kind_from_refname(const char *refname)
 {
unsigned int i;
@@ -1930,13 +1942,11 @@ static int ref_filter_handler(const char *refname, 
const struct object_id *oid,
 * to do its job and the resulting list may yet to be pruned
 * by maxcount logic.
 */
-   ref = new_ref_array_item(refname, oid);
+   ref = ref_array_push(ref_cbdata->array, refname, oid);
ref->commit = commit;
ref->flag = flag;
ref->kind = kind;
 
-   REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
-   ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
return 0;
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index 68268f9eb..76cf87cb6 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -135,4 +135,12 @@ void setup_ref_filter_porcelain_msg(void);
 void pretty_print_ref(const char *name, const struct object_id *oid,
  const struct ref_format *format);
 
+/*
+ * Push a single ref onto the array; this can be used to construct your own
+ * ref_array without using filter_refs().
+ */
+struct ref_array_item *ref_array_push(struct ref_array *array,
+ const char *refname,
+ const struct object_id *oid);
+
 #endif /*  REF_FILTER_H  */
-- 
2.14.3 (Apple Git-98)

Re: [PATCH v11 1/4] ref-filter: use "struct object_id" consistently

[Harald Nordgren]

Thanks for your very thorough review Eric! I think I address all the
points, but if I missed something please let me know.


On Sun, Apr 8, 2018 at 3:06 AM, Eric Sunshine  wrote:

>
> You incorrectly dropped Peff's sign-off[1] when re-sending the patches
> he authored in the series. And, your sign-off should follow his.

I just rebased on https://github.com/peff/git/tree/jk/ref-array-push
where there were no sign-off tags.

But I amended the commit messages now to add the sign-off. And also
added my own sign-off.

> Also, if you made any changes to Peff's patch, it's a good idea to
> state so with a bracketed comment at the end of the commit message
> (before the sign-offs). For instance:
>
> [hn: tweaked doodle blap]
>
> or such.
>
> [1]: 
> https://public-inbox.org/git/20180406185831.ga11...@sigill.intra.peff.net/

As a sign of respect I probably wouldn't want to tweak the patches.

They currently work well together with my implementation, so there is no need.

> This "last becomes primary key" feels counterintuitive to me, however,
> I see it mirrors precedence of other Git commands.
>
> In what situations would it make sense to specify --sort= multiple
> times in the context of ls-remote? If there are none, then I wonder if
> this should instead be documented as "last wins"...
>
> To what does "Also" refer?

This was copied wholesale from Documentation/git-tag.txt.

I minimized the text now and removed all the references to using
'sort' multiple times. Although you technically could use multiple
keys, since 'ls-remote' only outputs two columns, I guess it's never
needed.

> What does "not work" mean in this context? Will the command crash
> outright? Will it emit a suitable error message or warning? Will the
> sorting be somehow dysfunctional?

This will be the output when trying to sort by commiterdate:

From g...@github.com:git/git.git
fatal: missing object f0d0fd3a5985d5e588da1e1d11c85fba0ae132f8 for
refs/pull/10/head

I added a a note in the documentation saying that it will "cause a
failure". Should we be even more explicit than that?

This is one side-effect of borrowing the ref-filter logic in this
patch. We inherit things that won't work on remotes.

> It seems like the sentence "The keys supported..." should go above the
> "Also supports..." sentence for this explanation to be more cohesive.
>
> Finally, how about adding a linkgit:git-for-each-ref[1] to give the
> user an easy way to access the referenced documentation. For instance:
>
> The keys supported are the same as those accepted by the
> `--sort=` option of linkgit:git-for-each-ref[1], except...
>

Added a "linkgit" to git-for-each-ref. And a "See also" section at the bottom.

> Can we have a more meaningful name than 'array'? Even a name a simple
> as 'refs' would convey more information.

I think 'refs' would be confusing considering that the return value of
'transport_get_remote_refs' is stored as 'ref'. I renamed it to
'ref_array'. I hope it's not a problem that is shadows its struct
name. But I've seen us this naming paradigm in other places -- and in
this file.

> Do we need to worry about freeing memory allocated by these two lines?
>
> More generally, do we care that 'array' and 'sorting' are being
> leaked? If not, perhaps they should be annotated by UNLEAK.

Since 'cmd_ls_remote' is always (?) called from another process I
don't think we need to worry explicitly freeing the memory. I added
UNLEAK annotations to my code.

> This test script is already filled with these hardcoded SHA-1's, so
> this is not a new problem, but work is under way to make it possible
> to replace SHA-1 with some other hashing algorithm. Test scripts are
> being retrofitted to avoid hard-coding them; instead determining the
> hash value dynamically (for example, [1]). It would be nice if the new
> tests followed suit, thus saving someone else extra work down the
> road. (This, itself, is not worth a re-roll, but something to consider
> if you do re-roll.)
>
> [1]: 
> https://public-inbox.org/git/20180325192055.841459-10-sand...@crustytoothpaste.net/

Added 'git rev-parse' calls to my tests do decrease the reliance on
SHA-1's. For the test

ls-remote --symref

I couldn't figure out a way to extract the hash for
'refs/remotes/origin/HEAD' so I re-used HEAD. I tried different ways
of fetching origin, making another commit and pushing, but origin/HEAD
is still unavailable.

By calling 'git fetch origin' before the test, at least I am able to
extract 'git rev-parse refs/remotes/origin/master'.

This all makes the tests a bit more complicated, so I hope it helps us
in the future! :)

> Do we want a test verifying that multiple sort keys are respected? (Is
> it even possible to construct such a test?)

To add to my previous point: Since 'ls-remote' outputs only two
columns, I don't see a use case for multiple keys.

And I don't know what the test would look like either.

>> @@ -131,7 +167,7 @@ 

[PATCH v12 1/4] ref-filter: use "struct object_id" consistently

[Harald Nordgren]

From: Jeff King 

Internally we store a "struct object_id", and all of our
callers have one to pass us. But we insist that they peel it
to its bare-sha1 hash, which we then hashcpy() into place.
Let's pass it around as an object_id, which future-proofs us
for a post-sha1 world.

Signed-off-by: Jeff King 
Signed-off-by: Harald Nordgren 
---
 builtin/tag.c|  2 +-
 builtin/verify-tag.c |  2 +-
 ref-filter.c | 10 +-
 ref-filter.h |  2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index da186691e..42278f516 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -117,7 +117,7 @@ static int verify_tag(const char *name, const char *ref,
return -1;
 
if (format->format)
-   pretty_print_ref(name, oid->hash, format);
+   pretty_print_ref(name, oid, format);
 
return 0;
 }
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index ad7b79fa5..6fa04b751 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -72,7 +72,7 @@ int cmd_verify_tag(int argc, const char **argv, const char 
*prefix)
}
 
if (format.format)
-   pretty_print_ref(name, oid.hash, );
+   pretty_print_ref(name, , );
}
return had_error;
 }
diff --git a/ref-filter.c b/ref-filter.c
index 45fc56216..ade97a848 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1826,12 +1826,12 @@ static const struct object_id *match_points_at(struct 
oid_array *points_at,
 
 /* Allocate space for a new ref_array_item and copy the objectname and flag to 
it */
 static struct ref_array_item *new_ref_array_item(const char *refname,
-const unsigned char 
*objectname,
+const struct object_id *oid,
 int flag)
 {
struct ref_array_item *ref;
FLEX_ALLOC_STR(ref, refname, refname);
-   hashcpy(ref->objectname.hash, objectname);
+   oidcpy(>objectname, oid);
ref->flag = flag;
 
return ref;
@@ -1927,7 +1927,7 @@ static int ref_filter_handler(const char *refname, const 
struct object_id *oid,
 * to do its job and the resulting list may yet to be pruned
 * by maxcount logic.
 */
-   ref = new_ref_array_item(refname, oid->hash, flag);
+   ref = new_ref_array_item(refname, oid, flag);
ref->commit = commit;
 
REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
@@ -2165,11 +2165,11 @@ void show_ref_array_item(struct ref_array_item *info,
putchar('\n');
 }
 
-void pretty_print_ref(const char *name, const unsigned char *sha1,
+void pretty_print_ref(const char *name, const struct object_id *oid,
  const struct ref_format *format)
 {
struct ref_array_item *ref_item;
-   ref_item = new_ref_array_item(name, sha1, 0);
+   ref_item = new_ref_array_item(name, oid, 0);
ref_item->kind = ref_kind_from_refname(name);
show_ref_array_item(ref_item, format);
free_array_item(ref_item);
diff --git a/ref-filter.h b/ref-filter.h
index 0d98342b3..68268f9eb 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -132,7 +132,7 @@ void setup_ref_filter_porcelain_msg(void);
  * Print a single ref, outside of any ref-filter. Note that the
  * name must be a fully qualified refname.
  */
-void pretty_print_ref(const char *name, const unsigned char *sha1,
+void pretty_print_ref(const char *name, const struct object_id *oid,
  const struct ref_format *format);
 
 #endif /*  REF_FILTER_H  */
-- 
2.14.3 (Apple Git-98)

Re: [PATCH v7 07/14] commit-graph: implement git-commit-graph write

[Jakub Narebski]

Derrick Stolee  writes:

> +# Current graph structure:
> +#
> +#   __M3___
> +#  /   |   \
> +# 3 M1 5 M2 7
> +# |/  \|/  \|
> +# 246
> +# |___//
> +# 1

Good, so we are testing EDGE chunk, because the commit graph has octopus
merge in it (with more than two parents).

Do we need to test multiple roots, and/or independent (orphan) branches
cases?

Best,
-- 
Jakub Narębski

Re: something about git clone

[Ævar Arnfjörð Bjarmason]

On Sun, Apr 08 2018, jiangwei zhao wrote:

> hello!
>   i am a chinese.
>   i have some question for you ,i want to know how 'git clone'work,
> it likes downloader? or other things. i think it'sdown speed is
> heigher,so i email to you .
> thanks ,and my english is so poor.:P

You will not get the same speed from "git clone" as "wget" of a static
file, since the server may need to do a lot more work to serve you, so
it will be slower than downloading a static file.

Re: getting pull and push URLs for the current branch

[Ævar Arnfjörð Bjarmason]

On Sun, Apr 08 2018, Michal Novotny wrote:

> is there a way to get remote url  for the current branch? That is the
> url that will be used when I call `git pull`. It doesn't seem to be a
> particularly easy task.

You'd do something like this (sans error checking):

git remote get-url $(git config branch.$(git rev-parse --abbrev-ref 
HEAD).remote)

I.e. your HEAD may have remote tracking info set up, this'll get the
remote URL for the remote it points at.

Re: [PATCH v7 04/14] graph: add commit graph design document

[Jakub Narebski]

Derrick Stolee  writes:

> From: Derrick Stolee 
>
> Add Documentation/technical/commit-graph.txt with details of the planned
> commit graph feature, including future plans.

That's in my opinion a very good idea.  It would help anyone trying to
add to and extend this feature.

> Signed-off-by: Derrick Stolee 
> ---
>  Documentation/technical/commit-graph.txt | 163 +++
>  1 file changed, 163 insertions(+)
>  create mode 100644 Documentation/technical/commit-graph.txt
>
> diff --git a/Documentation/technical/commit-graph.txt 
> b/Documentation/technical/commit-graph.txt
> new file mode 100644
> index 00..0550c6d0dc
> --- /dev/null
> +++ b/Documentation/technical/commit-graph.txt
> @@ -0,0 +1,163 @@
> +Git Commit Graph Design Notes
> +=
> +
> +Git walks the commit graph for many reasons, including:
> +
> +1. Listing and filtering commit history.
> +2. Computing merge bases.
> +
> +These operations can become slow as the commit count grows. The merge
> +base calculation shows up in many user-facing commands, such as 'merge-base'
> +or 'status' and can take minutes to compute depending on history shape.
> +
> +There are two main costs here:
> +
> +1. Decompressing and parsing commits.
> +2. Walking the entire graph to satisfy topological order constraints.
> +
> +The commit graph file is a supplemental data structure that accelerates
> +commit graph walks. If a user downgrades or disables the 'core.commitGraph'
> +config setting, then the existing ODB is sufficient.

This is a good explanation of why we want to have this, and why in this
form.  I really like the "Related links" with summary.

>   The file is stored
> +as "commit-graph" either in the .git/objects/info directory or in the info
> +directory of an alternate.

That is a good thing.

Do I understand it correctly that Git would use first "commit-graph"
file that it would encounter, or would it merge information from all
alternates (including perhaps self)?  What if repository is a
subtree-merge of different repositories, with each listed separately as
alternate?

> +
> +The commit graph file stores the commit graph structure along with some
> +extra metadata to speed up graph walks. By listing commit OIDs in lexi-
> +cographic order, we can identify an integer position for each commit and
> +refer to the parents of a commit using those integer positions. We use
> +binary search to find initial commits and then use the integer positions
> +for fast lookups during the walk.

It might be worth emphasizing here that fast access using integer
positions for commits in the chunk is possible only if chunk used
fixed-width format (each commit taking the same amount of space -- which
for example is not true for packfile).

> +
> +A consumer may load the following info for a commit from the graph:
> +
> +1. The commit OID.
> +2. The list of parents, along with their integer position.
> +3. The commit date.
> +4. The root tree OID.
> +5. The generation number (see definition below).
> +
> +Values 1-4 satisfy the requirements of parse_commit_gently().

Good to have it here.  It is nice to know why 1-4 are needed to be in
the "commit-graph" structure.

Do I understand it correctly that this feature is what makes porting Git
to start using "commit-graph" information easy, because it is single
point of entry, isn't it?

> +
> +Define the "generation number" of a commit recursively as follows:
> +
> + * A commit with no parents (a root commit) has generation number one.
> +
> + * A commit with at least one parent has generation number one more than
> +   the largest generation number among its parents.
> +
> +Equivalently, the generation number of a commit A is one more than the
> +length of a longest path from A to a root commit. The recursive definition
> +is easier to use for computation and observing the following property:
> +
> +If A and B are commits with generation numbers N and M, respectively,
> +and N <= M, then A cannot reach B. That is, we know without searching
> +that B is not an ancestor of A because it is further from a root commit
> +than A.

Because generation numbers from the "commit-graph" may not cover all
commits (recent commits can have no generation number information), I
think it would be good idea to state what happens then.

Because of how generation numbers are defined, if commit A has
generation number provided, then all ancestors also have generation
number information.  Thus:

   If A is a commit with generation number N, and B is a commit
   without generation number information, then A cannot reach B.
   That is, we know without searching that B is not an ancestor of A
   because if it was, it would have generation number information.

Additionally (but that might be not worth adding here):

   If A is a commit without 

getting pull and push URLs for the current branch

[Michal Novotny]

Hello,

is there a way to get remote url  for the current branch? That is the
url that will be used when I call `git pull`. It doesn't seem to be a
particularly easy task.

Thank you!
clime

Re: [PATCH v6 6/6] worktree: teach "add" to check out existing branches

[Eric Sunshine]

On Sat, Mar 31, 2018 at 11:18 AM, Thomas Gummerer  wrote:
> [...]
> However we can do a little better than that, and check the branch out if
> it is not checked out anywhere else.  This will help users who just want
> to check an existing branch out into a new worktree, and save a few
> keystrokes.
> [...]
> Signed-off-by: Thomas Gummerer 
> ---
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> @@ -61,8 +61,13 @@ $ git worktree add --track -b   
> /
>  If `` is omitted and neither `-b` nor `-B` nor `--detach` used,
> +then, as a convenience, a worktree with a branch named after
> +`$(basename )` (call it ``) is created.

I had a hard time digesting this. I _think_ it wants to say:

If `` is omitted and neither `-b` nor `-B` nor
`--detach` is used, then, as a convenience, the new worktree is
associated with a branch (call it ``) named after
`$(basename )`.

>  If ``
> +doesn't exist, a new branch based on HEAD is automatically created as
> +if `-b ` was given.  If `` exists in the repository,

Maybe: s/exists in the repository/does exist/
Or: s/.../is a local branch/

Though, the latter may be getting too pedantic.

> +it will be checked out in the new worktree, if it's not checked out
> +anywhere else, otherwise the command will refuse to create the
> +worktree (unless `--force` is used).
> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> @@ -198,13 +198,24 @@ test_expect_success '"add" with  omitted' '
> +test_expect_success '"add" checks out existing branch of dwimd name' '
> +   git branch dwim HEAD~1 &&
> +   git worktree add dwim &&
> +   test_cmp_rev HEAD~1 dwim &&
> +   (
> +   cd dwim &&
> +   test_cmp_rev dwim HEAD

Nit: Argument order of the two test_cmp_rev() invocations differs.

> +   )
> +'
> +
> +test_expect_success '"add " dwim fails with checked out branch' '
> +   git checkout -b test-branch &&
> +   test_must_fail git worktree add test-branch &&
> +   test_path_is_missing test-branch
> +'
> +
> +test_expect_success '"add --force" with existing dwimd name doesnt die' '
> +   git worktree add --force test-branch
>  '

Maybe make this last test slightly more robust by having it "git
checkout test-branch" before "git worktree add ..." to protect against
someone inserting a new test (which checks out some other branch)
between these two. Probably not worth a re-roll.

Re: [PATCH v1 1/2] perf/run: add --subsection option

[Eric Sunshine]

On Sun, Apr 8, 2018 at 5:35 AM, Christian Couder
 wrote:
> This new option makes it possible to run perf tests as defined
> in only one subsection of a config file.
>
> Signed-off-by: Christian Couder 
> ---
> diff --git a/t/perf/run b/t/perf/run
> @@ -1,21 +1,34 @@
> +while [ $# -gt 0 ]; do

I was going to ask if you meant to use 'test' here rather than '[',
however, I see that this script already has a mix of the two, so...

Likewise, scripts in Git usually omit the semicolon and place 'do' on
its own line, however, again I see that this script has a mix of the
two styles, so...

[PATCH v1 1/2] perf/run: add --subsection option

[Christian Couder]

This new option makes it possible to run perf tests as defined
in only one subsection of a config file.

Signed-off-by: Christian Couder 
---
 t/perf/run | 56 --
 1 file changed, 46 insertions(+), 10 deletions(-)

diff --git a/t/perf/run b/t/perf/run
index 213da5d6b9..9aaa733c77 100755
--- a/t/perf/run
+++ b/t/perf/run
@@ -1,21 +1,34 @@
 #!/bin/sh
 
-case "$1" in
+die () {
+   echo >&2 "error: $*"
+   exit 1
+}
+
+while [ $# -gt 0 ]; do
+   arg="$1"
+   case "$arg" in
+   --)
+   break ;;
--help)
-   echo "usage: $0 [--config file] [other_git_tree...] [--] 
[test_scripts]"
-   exit 0
-   ;;
+   echo "usage: $0 [--config file] [--subsection subsec] 
[other_git_tree...] [--] [test_scripts]"
+   exit 0 ;;
--config)
shift
GIT_PERF_CONFIG_FILE=$(cd "$(dirname "$1")"; pwd)/$(basename 
"$1")
export GIT_PERF_CONFIG_FILE
shift ;;
-esac
-
-die () {
-   echo >&2 "error: $*"
-   exit 1
-}
+   --subsection)
+   shift
+   GIT_PERF_SUBSECTION="$1"
+   export GIT_PERF_SUBSECTION
+   shift ;;
+   --*)
+   die "unrecognised option: '$arg'" ;;
+   *)
+   break ;;
+   esac
+done
 
 run_one_dir () {
if test $# -eq 0; then
@@ -172,9 +185,32 @@ get_subsections "perf" >test-results/run_subsections.names
 
 if test $(wc -l /dev/null ||
+   die "subsection '$GIT_PERF_SUBSECTION' not found in 
'$GIT_PERF_CONFIG_FILE'"
+
+   egrep "^$GIT_PERF_SUBSECTION\$" test-results/run_subsections.names | 
while read -r subsec
+   do
+   (
+   GIT_PERF_SUBSECTION="$subsec"
+   export GIT_PERF_SUBSECTION
+   echo " Run for subsection 
'$GIT_PERF_SUBSECTION' "
+   run_subsection "$@"
+   )
+   done
 else
while read -r subsec
do
-- 
2.17.0.rc1.36.g098d832c9.dirty

[PATCH v1 0/2] t/perf: bisect performance regressions

[Christian Couder]

This is a small patch series on top of cc/perf-aggregate-sort, which
is next, to add scripts that make it much easier to bisect performance
regressions.

For example if you ran perf test using the perf.conf config file that
contains a "with libpcre", then using:

$ ./aggregate.perl --subsection "with libpcre" --sort-by=regression v2.15.1 
v2.16.2 v2.17.0 

you can get a line in the output like:

+8.2% p5302-pack-index.6 14.33(44.50+0.77) 15.50(43.83+0.94) v2.16.2 v2.17.0

and you can now use this output line to bisect where the regression
comes from using:

echo "+8.2% p5302-pack-index.6 14.33(44.50+0.77) 15.50(43.83+0.94) v2.16.2 
v2.17.0" | ./bisect_regression --config perf.conf --subsection "with libpcre"

Caveats:

You must make sure that the times are consistent between runs and that
there is a significant time difference between the "good" runs and the
"bad" runs.

For example the following is not easily bisectable:

+100.0% p-perf-lib-sanity.2 0.01(0.01+0.01) 0.02(0.02+0.00) v2.15.1 v2.16.2

as the rtime went from 0.01 s to 0.02 s and we only have 0.01 s precision.

Christian Couder (2):
  perf/run: add --subsection option
  t/perf: add scripts to bisect performance regressions

 t/perf/bisect_regression | 73 
 t/perf/bisect_run_script | 47 ++
 t/perf/run   | 56 --
 3 files changed, 166 insertions(+), 10 deletions(-)
 create mode 100755 t/perf/bisect_regression
 create mode 100755 t/perf/bisect_run_script

-- 
2.17.0.rc1.36.g098d832c9.dirty

[PATCH v1 2/2] t/perf: add scripts to bisect performance regressions

[Christian Couder]

The new bisect_regression script can be used to automatically bisect
performance regressions. It will pass the new bisect_run_script to
`git bisect run`.

Signed-off-by: Christian Couder 
---
 t/perf/bisect_regression | 73 
 t/perf/bisect_run_script | 47 ++
 2 files changed, 120 insertions(+)
 create mode 100755 t/perf/bisect_regression
 create mode 100755 t/perf/bisect_run_script

diff --git a/t/perf/bisect_regression b/t/perf/bisect_regression
new file mode 100755
index 00..a94d9955d0
--- /dev/null
+++ b/t/perf/bisect_regression
@@ -0,0 +1,73 @@
+#!/bin/sh
+
+# Read a line coming from `./aggregate.perl --sort-by regression ...`
+# and automatically bisect to find the commit responsible for the
+# performance regression.
+#
+# Lines from `./aggregate.perl --sort-by regression ...` look like:
+#
+# +100.0% p7821-grep-engines-fixed.1 0.04(0.10+0.03) 0.08(0.11+0.08) v2.14.3 
v2.15.1
+# +33.3% p7820-grep-engines.1 0.03(0.08+0.02) 0.04(0.08+0.02) v2.14.3 v2.15.1
+#
+
+die () {
+   echo >&2 "error: $*"
+   exit 1
+}
+
+while [ $# -gt 0 ]; do
+   arg="$1"
+   case "$arg" in
+   --help)
+   echo "usage: $0 [--config file] [--subsection subsection]"
+   exit 0
+   ;;
+   --config)
+   shift
+   GIT_PERF_CONFIG_FILE=$(cd "$(dirname "$1")"; pwd)/$(basename 
"$1")
+   export GIT_PERF_CONFIG_FILE
+   shift ;;
+   --subsection)
+   shift
+   GIT_PERF_SUBSECTION="$1"
+   export GIT_PERF_SUBSECTION
+   shift ;;
+   --*)
+   die "unrecognised option: '$arg'" ;;
+   *)
+   die "unknown argument '$arg'"
+   ;;
+   esac
+done
+
+read -r regression subtest oldtime newtime oldrev newrev
+
+test_script=$(echo "$subtest" | sed -e 's/\(.*\)\.[0-9]*$/\1.sh/')
+test_number=$(echo "$subtest" | sed -e 's/.*\.\([0-9]*\)$/\1/')
+
+# oldtime and newtime are decimal number, not integers
+
+oldtime=$(echo "$oldtime" | sed -e 's/^\([0-9]\+\.[0-9]\+\).*$/\1/')
+newtime=$(echo "$newtime" | sed -e 's/^\([0-9]\+\.[0-9]\+\).*$/\1/')
+
+test $(echo "$newtime" "$oldtime" | awk '{ print ($1 > $2) }') = 1 ||
+   die "New time '$newtime' shoud be greater than old time '$oldtime'"
+
+tmpdir=$(mktemp -d -t bisect_regression_XX) || die "Failed to create temp 
directory"
+echo "$oldtime" >"$tmpdir/oldtime" || die "Failed to write to 
'$tmpdir/oldtime'"
+echo "$newtime" >"$tmpdir/newtime" || die "Failed to write to 
'$tmpdir/newtime'"
+
+# Bisecting must be performed from the top level directory (even with 
--no-checkout)
+(
+   toplevel_dir=$(git rev-parse --show-toplevel) || die "Failed to find 
top level directory"
+   cd "$toplevel_dir" || die "Failed to cd into top level directory 
'$toplevel_dir'"
+
+   git bisect start --no-checkout "$newrev" "$oldrev" || die "Failed to 
start bisecting"
+
+   git bisect run t/perf/bisect_run_script "$test_script" "$test_number" 
"$tmpdir"
+   res="$?"
+
+   git bisect reset
+
+   exit "$res"
+)
diff --git a/t/perf/bisect_run_script b/t/perf/bisect_run_script
new file mode 100755
index 00..038255df4b
--- /dev/null
+++ b/t/perf/bisect_run_script
@@ -0,0 +1,47 @@
+#!/bin/sh
+
+script="$1"
+test_number="$2"
+info_dir="$3"
+
+# This aborts the bisection immediately
+die () {
+   echo >&2 "error: $*"
+   exit 255
+}
+
+bisect_head=$(git rev-parse --verify BISECT_HEAD) || die "Failed to find 
BISECT_HEAD ref"
+
+script_number=$(echo "$script" | sed -e "s/^p\([0-9]*\).*\$/\1/") || die 
"Failed to get script number for '$script'"
+
+oldtime=$(cat "$info_dir/oldtime") || die "Failed to access 
'$info_dir/oldtime'"
+newtime=$(cat "$info_dir/newtime") || die "Failed to access 
'$info_dir/newtime'"
+
+cd t/perf || die "Failed to cd into 't/perf'"
+
+result_file="$info_dir/perf_${script_number}_${bisect_head}_results.txt"
+
+GIT_PERF_DIRS_OR_REVS="$bisect_head"
+export GIT_PERF_DIRS_OR_REVS
+
+./run "$script" >"$result_file" 2>&1 || die "Failed to run perf test '$script'"
+
+rtime=$(sed -n 
"s/^$script_number\.$test_number:.*\([0-9]\+\.[0-9]\+\)(.*).*\$/\1/p" 
"$result_file")
+
+echo "newtime: $newtime"
+echo "rtime: $rtime"
+echo "oldtime: $oldtime"
+
+# Compare ($newtime - $rtime) with ($rtime - $oldtime)
+# Times are decimal number, not integers
+
+if test $(echo "$newtime" "$rtime" "$oldtime" | awk '{ print ($1 - $2 > $2 - 
$3) }') = 1
+then
+   # Current commit is considered "good/old"
+   echo "$rtime" >"$info_dir/oldtime"
+   exit 0
+else
+   # Current commit is considered "bad/new"
+   echo "$rtime" >"$info_dir/newtime"
+   exit 1
+fi
-- 
2.17.0.rc1.36.g098d832c9.dirty

Re: [PATCH v6 3/6] worktree: improve message when creating a new worktree

[Eric Sunshine]

On Sat, Mar 31, 2018 at 11:18 AM, Thomas Gummerer  wrote:
> [...]
> Fix these inconsistencies, and no longer show the identifier by making
> the 'git reset --hard' call not print the "HEAD is now at ..." line
> using the newly introduced flag from the previous commit, and printing
> the message directly from the builtin command instead.
>
> Signed-off-by: Thomas Gummerer 
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -322,12 +320,22 @@ static int add_worktree(const char *path, const char 
> *refname,
> argv_array_pushl(, "reset", "--hard", NULL);
> +   argv_array_push(, "--no-show-new-head-line");
> cp.env = child_env.argv;
> ret = run_command();
> if (ret)
> goto done;
> }
>
> +   fprintf(stderr, _("New worktree HEAD is now at %s"),
> +   find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));
> +
> +   strbuf_reset();
> +   pp_commit_easy(CMIT_FMT_ONELINE, commit, );
> +   if (sb.len > 0)
> +   fprintf(stderr, " %s", sb.buf);
> +   fputc('\n', stderr);
> +
> is_junk = 0;
> FREE_AND_NULL(junk_work_tree);
> FREE_AND_NULL(junk_git_dir);

Generally speaking, code such as this probably ought to be inserted
outside of the is_junk={1,0} context in order to keep that critical
section as small as possible. However, as mentioned in my response to
the v6 cover letter[1], I think this chunk of new code can just go
away entirely if git-reset is made to print the customized message on
git-worktree's behalf.

[1]: 
https://public-inbox.org/git/capig+cq8vzdycumo-qoexndbgqgegj2bpmpa-y0vhgct_br...@mail.gmail.com/

Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches

[Eric Sunshine]

On Sat, Mar 31, 2018 at 11:17 AM, Thomas Gummerer  wrote:
> This round should fix all the UI issues Eric found in the last round.
> The changes I made in a bit more detail:
>
> - added a new commit introducing a new hidden --show-new-head-line
>   flag in 'git reset'.  This is used to suppress the "HEAD is now at
>   ..."  line that 'git reset --hard' usually prints, so we can replace
>   it with our own "New worktree HEAD is now at ..." line instead,
>   while keeping the progress indicator for larger repositories.

As with Junio, I'm fine with this hidden option (for now), however, I
think you can take this a step further. Rather than having a (hidden)
git-reset option which suppresses "HEAD is now at...", instead have a
(hidden) option which augments the message. For example,
--new-head-desc="New worktree" would make it output "New worktree HEAD
is now at...". Changes to builtin/reset.c to support this would hardly
be larger than the changes you already made.

The major benefit is that patch 3/6 no longer has to duplicate the
code from builtin/reset.c:print_new_head_line() just to print its own
"New worktree HEAD is now at..." message. (As for the argument that
"git worktree add" must duplicate that code because it wants the
message on stderr, whereas git-reset prints it to stdout, I don't see
why git-worktree puts those messages to stderr in the first place. As
far as I can tell, it would be equally valid to print them to stdout.)

> Some examples of the new UI behaviour here for reference:
>
>  - guess-remote mode
>
> $ git worktree add --guess-remote ../next
> Creating branch 'next'
> Branch 'next' set up to track remote branch 'next' from 'origin'.
> New worktree HEAD is now at caa68db14 Merge branch 
> 'sb/packfiles-in-repository' into next
>
>  - original dwim (create a branch based on the current HEAD)
>
> $ git worktree add ../test
> Creating branch 'test'
> New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'
>
>  - new dwim (check out existing branch)
>
> $ git worktree add ../test
> Checking out branch 'test'
> New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'
>
>  - no new branch created
>
> $ git worktree add ../test2 origin/master
> New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'

I like the "creating" or "checking out" messages we now get for all
the DWIM cases. I wonder if it would make sense to print "Checkout out
blah..." for this case too. It's certainly not necessary since the
user specified  explicitly, but it would make the UI even
more consistent, and address your subsequent comment about missing
context above the "Checking out files: ...%" line for this case.
Thoughts?

> Compare this to the old UI (new dwim omitted, as there's no old
> version of that):

Thanks for contrasting the new with the old. The new output is nicer
and more helpful.

> The one thing we are loosing is a context line before "Checking out
> files:", if no new branch is created.  Personally I feel like that's
> acceptable, as the user just used the 'git worktree add' command, so
> it should be intuitive where those files are being checked out.

something about git clone

[jiangwei zhao]

hello!
  i am a chinese.
  i have some question for you ,i want to know how 'git clone'work,
it likes downloader? or other things. i think it'sdown speed is
heigher,so i email to you .
thanks ,and my english is so poor.:P