Re: [PATCH 2/8] sequencer: introduce the `merge` command

[Jacob Keller]

On Wed, Jan 31, 2018 at 5:48 AM, Johannes Schindelin
 wrote:
> Hi Jake & Phillip,
>
> On Mon, 29 Jan 2018, Johannes Schindelin wrote:
>
>> On Sat, 20 Jan 2018, Jacob Keller wrote:
>>
>> > On Fri, Jan 19, 2018 at 6:45 AM, Phillip Wood  
>> > wrote:
>> > > On 18/01/18 15:35, Johannes Schindelin wrote:
>> > >>
>> > >> This patch adds the `merge` command, with the following syntax:
>> > >>
>> > >>   merge   
>> > >
>> > > I'm concerned that this will be confusing for users. All of the other
>> > > rebase commands replay the changes in the commit hash immediately
>> > > following the command name. This command instead uses the first
>> > > commit to specify the message which is different to both 'git merge'
>> > > and the existing rebase commands. I wonder if it would be clearer to
>> > > have 'merge -C   ...' instead so it's clear which
>> > > argument specifies the message and which the remote head to merge.
>> > > It would also allow for 'merge -c   ...' in the future
>> > > for rewording an existing merge message and also avoid the slightly
>> > > odd 'merge -  ...'. Where it's creating new merges I'm not sure
>> > > it's a good idea to encourage people to only have oneline commit
>> > > messages by making it harder to edit them, perhaps it could take
>> > > another argument to mean open the editor or not, though as Jake said
>> > > I guess it's not that common.
>> >
>> > I actually like the idea of re-using commit message options like -C,
>> > -c,  and -m, so we could do:
>> >
>> > merge -C  ... to take message from commit
>>
>> That is exactly how the Git garden shears do it.
>>
>> I found it not very readable. That is why I wanted to get away from it in
>> --recreate-merges.
>
> I made up my mind. Even if it is not very readable, it is still better
> than the `merge A B` where the order of A and B magically determines their
> respective roles.
>
>> > merge -c  ...  to take the message from commit and open editor to 
>> > edit
>> > merge -m "" ... to take the message from the quoted test
>> > merge ... to merge and open commit editor with default message
>
> I will probably implement -c, but not -m, and will handle the absence of
> the -C and -c options to construct a default merge message which can then
> be edited.
>
> The -m option just opens such a can of worms with dequoting, that's why I
> do not want to do that.
>

I agree, I don't see a need for "-m".

> BTW I am still trying to figure out how to present the oneline of the
> commit to merge (which is sometimes really helpful because the label might
> be less than meaningful) while *still* allowing for octopus merges.
>
> So far, what I have is this:
>
> merge   
>
> and for octopus:
>
> merge  " ..." ...
>
> I think with the -C syntax, it would become something like
>
> merge -C   # 
>

I like this, especially given you added the "#" for one of the other
new commands as well, (reset I think?)

> and
>
> merge -C   ...
> # Merging: 
> # Merging: 
> # ...
>

I really like this, since you can show each oneline for all the
to-merges for an octopus.

> The only qualm I have about this is that `#` really *is* a valid ref name.
> (Seriously, it is...). So that would mean that I'd have to disallow `#`
> as a label specifically.
>
> Thoughts?
>

I think it's fine to disallow # as a label.

Thanks,
Jake

> Ciao,
> Dscho

Re: [PATCH v2 00/10] rebase -i: offer to recreate merge commits

[Jacob Keller]

On Wed, Jan 31, 2018 at 5:29 AM, Johannes Schindelin
 wrote:
> I think I may want to introduce a bigger change, still. I forgot who
> exactly came up with the suggestion to use `merge -C 
> ` (I think it was Jake), and I reacted too forcefully in
> rejecting it.
>

I believe someone else suggested it, but I replied that I liked it.

> This design had been my original design in the Git garden shears, and I
> did not like it because it felt clunky and it also broke the style of
>  .
>

I agree it's a bit weird it breaks the style of " ",
but on some level merge does this anyways as it's the first one to
take more than one argument.

> But the longer I think about this, the more I come to the conclusion that
> I was wrong, and that the -C way is the way that leaves the door open to
> the pretty elegant `-c ` (imitating `git commit`'s option to
> borrow the commit message from elsewhere but still allowing to edit it).
>

The other reason I liked this, is that it matches merge syntax on the
command line, so users don't need to learn a special new syntax for
the todo file.

> And it also leaves open the door to just write `merge ` and have
> the sequencer come up with a default merge message that the user can then
> edit.

I like that we could completely forgo the -C and -c in order to allow
the normal default merge commit message that is auto generated as
well.

>
> I'll probably refrain from implementing support for -m because I do not
> want to implement the dequoting.
>

Yea, I don't think that is necessary either.

Thanks,
Jake

> Ciao,
> Dscho

Assalam Alaikum

[Eiman Yousef M A Al-muzafar]

Assalam Alaikum, I am Eiman Yousef M A Al-muzafar, a Muslim woman from Qatar, I 
am contacting you regarding a relationship of trust and confidence for an 
inheritance. Please contact me on my private email for more details:  
eiman.y...@hotmail.com

Re: [PATCH v2 00/14] Serialized Git Commit Graph

[Stefan Beller]

>> Finally, I tried to clean up my incorrect style as I was recreating
>> these commits. Feel free to be merciless in style feedback now that the
>> architecture is more stable.
>
> ok, will do.

I reviewed all patches and found no nits worth mentioning.
The whole series has been reviewed by me.

>
> Thanks,
> Stefan

Re: [PATCH 0/2] Add "git rebase --show-patch"

[Tim Landscheidt]

Junio C Hamano  wrote:

>> The pseudo ref certainly has an appeal. For people very familiar with
>> Git's peculiarities such as FETCH_HEAD. Such as myself.

>> For users, it is probably substantially worse an experience than having a
>> cmdmode like --show-patch in the very command they were just running.

> I tend to agree with that assessment.  FETCH_HEAD was a required
> mechanism for commands in the toolchain to communicate and wasn't
> meant as a mechanism for end-users.  I do not think it is a good
> idea to add to the pile to these special files the users would need
> to look at, when we do not need to.

> Even if the internal implementation uses such a file, wrapping it
> with a documented command mode would make a better UI.

I disagree with that as I had:

| [alias]
| original-commit = !git show $(cat .git/rebase-apply/original-commit)

for a long time in my ~/.gitconfig :-) (until I realized
that Git accepted "rebase-apply/original-commit" everywhere;
for single-commit branches I always just used ORIG_HEAD).

This meant that whenever I wanted to look at the lay of the
land upon which the original commit was built, I had to do
"git original-commit" and copy & paste the SHA1 (without my
alias, I had to "git log ORIG_HEAD", pick hopefully the cor-
rect commit and copy & paste its SHA1).  Only with this SHA1
I could then run "git diff", "git show", "git grep", "git
blame", etc., etc., etc. because in my use cases looking at
the patch alone was usually not sufficient: I needed to know
why there was a conflict, i. e. how the file(s) the patch
changed looked before they had been altered upstream.

To me, this type of "algebra" is what makes Git so powerful:
I do not have to build a pipe that gets the SHA1 of a
branch's tip and xargs it to "git show", I can just say "git
show $branch".  Having a SHA1 with a special meaning that
has no easy way to access it by "git rev-parse" breaks this
UI IMHO.

Tim

[PATCH v2 05/12] sha1_file: switch uses of SHA-1 to the_hash_algo

[brian m. carlson]

Switch various uses of explicit calls to SHA-1 into references to
the_hash_algo for better abstraction.  Convert some calls to use struct
object_id.

Signed-off-by: brian m. carlson 
---
 sha1_file.c | 52 ++--
 1 file changed, 26 insertions(+), 26 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index ec6ecea170..ff55fb303d 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -786,16 +786,16 @@ void *xmmap(void *start, size_t length,
 int check_sha1_signature(const unsigned char *sha1, void *map,
 unsigned long size, const char *type)
 {
-   unsigned char real_sha1[20];
+   struct object_id real_oid;
enum object_type obj_type;
struct git_istream *st;
-   git_SHA_CTX c;
+   git_hash_ctx c;
char hdr[32];
int hdrlen;
 
if (map) {
-   hash_sha1_file(map, size, type, real_sha1);
-   return hashcmp(sha1, real_sha1) ? -1 : 0;
+   hash_sha1_file(map, size, type, real_oid.hash);
+   return hashcmp(sha1, real_oid.hash) ? -1 : 0;
}
 
st = open_istream(sha1, _type, , NULL);
@@ -806,8 +806,8 @@ int check_sha1_signature(const unsigned char *sha1, void 
*map,
hdrlen = xsnprintf(hdr, sizeof(hdr), "%s %lu", typename(obj_type), 
size) + 1;
 
/* Sha1.. */
-   git_SHA1_Init();
-   git_SHA1_Update(, hdr, hdrlen);
+   the_hash_algo->init_fn();
+   the_hash_algo->update_fn(, hdr, hdrlen);
for (;;) {
char buf[1024 * 16];
ssize_t readlen = read_istream(st, buf, sizeof(buf));
@@ -818,11 +818,11 @@ int check_sha1_signature(const unsigned char *sha1, void 
*map,
}
if (!readlen)
break;
-   git_SHA1_Update(, buf, readlen);
+   the_hash_algo->update_fn(, buf, readlen);
}
-   git_SHA1_Final(real_sha1, );
+   the_hash_algo->final_fn(real_oid.hash, );
close_istream(st);
-   return hashcmp(sha1, real_sha1) ? -1 : 0;
+   return hashcmp(sha1, real_oid.hash) ? -1 : 0;
 }
 
 int git_open_cloexec(const char *name, int flags)
@@ -1421,16 +1421,16 @@ static void write_sha1_file_prepare(const void *buf, 
unsigned long len,
 const char *type, unsigned char *sha1,
 char *hdr, int *hdrlen)
 {
-   git_SHA_CTX c;
+   git_hash_ctx c;
 
/* Generate the header */
*hdrlen = xsnprintf(hdr, *hdrlen, "%s %lu", type, len)+1;
 
/* Sha1.. */
-   git_SHA1_Init();
-   git_SHA1_Update(, hdr, *hdrlen);
-   git_SHA1_Update(, buf, len);
-   git_SHA1_Final(sha1, );
+   the_hash_algo->init_fn();
+   the_hash_algo->update_fn(, hdr, *hdrlen);
+   the_hash_algo->update_fn(, buf, len);
+   the_hash_algo->final_fn(sha1, );
 }
 
 /*
@@ -1552,8 +1552,8 @@ static int write_loose_object(const unsigned char *sha1, 
char *hdr, int hdrlen,
int fd, ret;
unsigned char compressed[4096];
git_zstream stream;
-   git_SHA_CTX c;
-   unsigned char parano_sha1[20];
+   git_hash_ctx c;
+   struct object_id parano_oid;
static struct strbuf tmp_file = STRBUF_INIT;
const char *filename = sha1_file_name(sha1);
 
@@ -1569,14 +1569,14 @@ static int write_loose_object(const unsigned char 
*sha1, char *hdr, int hdrlen,
git_deflate_init(, zlib_compression_level);
stream.next_out = compressed;
stream.avail_out = sizeof(compressed);
-   git_SHA1_Init();
+   the_hash_algo->init_fn();
 
/* First header.. */
stream.next_in = (unsigned char *)hdr;
stream.avail_in = hdrlen;
while (git_deflate(, 0) == Z_OK)
; /* nothing */
-   git_SHA1_Update(, hdr, hdrlen);
+   the_hash_algo->update_fn(, hdr, hdrlen);
 
/* Then the data itself.. */
stream.next_in = (void *)buf;
@@ -1584,7 +1584,7 @@ static int write_loose_object(const unsigned char *sha1, 
char *hdr, int hdrlen,
do {
unsigned char *in0 = stream.next_in;
ret = git_deflate(, Z_FINISH);
-   git_SHA1_Update(, in0, stream.next_in - in0);
+   the_hash_algo->update_fn(, in0, stream.next_in - in0);
if (write_buffer(fd, compressed, stream.next_out - compressed) 
< 0)
die("unable to write sha1 file");
stream.next_out = compressed;
@@ -1596,8 +1596,8 @@ static int write_loose_object(const unsigned char *sha1, 
char *hdr, int hdrlen,
ret = git_deflate_end_gently();
if (ret != Z_OK)
die("deflateEnd on object %s failed (%d)", sha1_to_hex(sha1), 
ret);
-   git_SHA1_Final(parano_sha1, );
-   if (hashcmp(sha1, parano_sha1) != 0)
+   the_hash_algo->final_fn(parano_oid.hash, );
+   if (hashcmp(sha1, parano_oid.hash) != 0)

[PATCH v2 10/12] csum-file: rename sha1file to hashfile

[brian m. carlson]

Rename struct sha1file to struct hashfile, along with all of its related
functions.

The transformation in this commit was made by global search-and-replace.
---
 builtin/index-pack.c   | 20 +--
 builtin/pack-objects.c | 52 +-
 bulk-checkin.c | 16 
 csum-file.c| 36 +-
 csum-file.h| 34 -
 fast-import.c  | 28 +--
 pack-bitmap-write.c| 30 ++---
 pack-write.c   | 32 +++
 pack.h |  4 ++--
 9 files changed, 126 insertions(+), 126 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 40c000aca8..0bb520c731 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1241,7 +1241,7 @@ static void resolve_deltas(void)
  * - append objects to convert thin pack to full pack if required
  * - write the final pack hash
  */
-static void fix_unresolved_deltas(struct sha1file *f);
+static void fix_unresolved_deltas(struct hashfile *f);
 static void conclude_pack(int fix_thin_pack, const char *curr_pack, unsigned 
char *pack_hash)
 {
if (nr_ref_deltas + nr_ofs_deltas == nr_resolved_deltas) {
@@ -1252,7 +1252,7 @@ static void conclude_pack(int fix_thin_pack, const char 
*curr_pack, unsigned cha
}
 
if (fix_thin_pack) {
-   struct sha1file *f;
+   struct hashfile *f;
unsigned char read_hash[GIT_MAX_RAWSZ], 
tail_hash[GIT_MAX_RAWSZ];
struct strbuf msg = STRBUF_INIT;
int nr_unresolved = nr_ofs_deltas + nr_ref_deltas - 
nr_resolved_deltas;
@@ -1262,7 +1262,7 @@ static void conclude_pack(int fix_thin_pack, const char 
*curr_pack, unsigned cha
REALLOC_ARRAY(objects, nr_objects + nr_unresolved + 1);
memset(objects + nr_objects + 1, 0,
   nr_unresolved * sizeof(*objects));
-   f = sha1fd(output_fd, curr_pack);
+   f = hashfd(output_fd, curr_pack);
fix_unresolved_deltas(f);
strbuf_addf(, Q_("completed with %d local object",
 "completed with %d local objects",
@@ -1270,7 +1270,7 @@ static void conclude_pack(int fix_thin_pack, const char 
*curr_pack, unsigned cha
nr_objects - nr_objects_initial);
stop_progress_msg(, msg.buf);
strbuf_release();
-   sha1close(f, tail_hash, 0);
+   hashclose(f, tail_hash, 0);
hashcpy(read_hash, pack_hash);
fixup_pack_header_footer(output_fd, pack_hash,
 curr_pack, nr_objects,
@@ -1286,7 +1286,7 @@ static void conclude_pack(int fix_thin_pack, const char 
*curr_pack, unsigned cha
nr_ofs_deltas + nr_ref_deltas - nr_resolved_deltas);
 }
 
-static int write_compressed(struct sha1file *f, void *in, unsigned int size)
+static int write_compressed(struct hashfile *f, void *in, unsigned int size)
 {
git_zstream stream;
int status;
@@ -1300,7 +1300,7 @@ static int write_compressed(struct sha1file *f, void *in, 
unsigned int size)
stream.next_out = outbuf;
stream.avail_out = sizeof(outbuf);
status = git_deflate(, Z_FINISH);
-   sha1write(f, outbuf, sizeof(outbuf) - stream.avail_out);
+   hashwrite(f, outbuf, sizeof(outbuf) - stream.avail_out);
} while (status == Z_OK);
 
if (status != Z_STREAM_END)
@@ -1310,7 +1310,7 @@ static int write_compressed(struct sha1file *f, void *in, 
unsigned int size)
return size;
 }
 
-static struct object_entry *append_obj_to_pack(struct sha1file *f,
+static struct object_entry *append_obj_to_pack(struct hashfile *f,
   const unsigned char *sha1, void *buf,
   unsigned long size, enum object_type type)
 {
@@ -1327,7 +1327,7 @@ static struct object_entry *append_obj_to_pack(struct 
sha1file *f,
}
header[n++] = c;
crc32_begin(f);
-   sha1write(f, header, n);
+   hashwrite(f, header, n);
obj[0].size = size;
obj[0].hdr_size = n;
obj[0].type = type;
@@ -1335,7 +1335,7 @@ static struct object_entry *append_obj_to_pack(struct 
sha1file *f,
obj[1].idx.offset = obj[0].idx.offset + n;
obj[1].idx.offset += write_compressed(f, buf, size);
obj[0].idx.crc32 = crc32_end(f);
-   sha1flush(f);
+   hashflush(f);
hashcpy(obj->idx.oid.hash, sha1);
return obj;
 }
@@ -1347,7 +1347,7 @@ static int delta_pos_compare(const void *_a, const void 
*_b)
return a->obj_no - b->obj_no;
 }
 
-static void fix_unresolved_deltas(struct sha1file *f)
+static void fix_unresolved_deltas(struct hashfile *f)
 {
struct 

[PATCH v2 09/12] read-cache: abstract away uses of SHA-1

[brian m. carlson]

Convert various uses of direct calls to SHA-1 and 20- and 40-based
constants to use the_hash_algo instead.  Don't yet convert the on-disk
data structures, which will be handled in a future commit.

Adjust some comments so as not to refer explicitly to SHA-1.

Signed-off-by: brian m. carlson 
---
 read-cache.c | 58 +-
 1 file changed, 29 insertions(+), 29 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 2eb81a66b9..15bbb9bab4 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1545,8 +1545,8 @@ int verify_ce_order;
 
 static int verify_hdr(struct cache_header *hdr, unsigned long size)
 {
-   git_SHA_CTX c;
-   unsigned char sha1[20];
+   git_hash_ctx c;
+   unsigned char hash[GIT_MAX_RAWSZ];
int hdr_version;
 
if (hdr->hdr_signature != htonl(CACHE_SIGNATURE))
@@ -1558,10 +1558,10 @@ static int verify_hdr(struct cache_header *hdr, 
unsigned long size)
if (!verify_index_checksum)
return 0;
 
-   git_SHA1_Init();
-   git_SHA1_Update(, hdr, size - 20);
-   git_SHA1_Final(sha1, );
-   if (hashcmp(sha1, (unsigned char *)hdr + size - 20))
+   the_hash_algo->init_fn();
+   the_hash_algo->update_fn(, hdr, size - the_hash_algo->rawsz);
+   the_hash_algo->final_fn(hash, );
+   if (hashcmp(hash, (unsigned char *)hdr + size - the_hash_algo->rawsz))
return error("bad index file sha1 signature");
return 0;
 }
@@ -1791,7 +1791,7 @@ int do_read_index(struct index_state *istate, const char 
*path, int must_exist)
die_errno("cannot stat the open index");
 
mmap_size = xsize_t(st.st_size);
-   if (mmap_size < sizeof(struct cache_header) + 20)
+   if (mmap_size < sizeof(struct cache_header) + the_hash_algo->rawsz)
die("index file smaller than expected");
 
mmap = xmmap(NULL, mmap_size, PROT_READ, MAP_PRIVATE, fd, 0);
@@ -1803,7 +1803,7 @@ int do_read_index(struct index_state *istate, const char 
*path, int must_exist)
if (verify_hdr(hdr, mmap_size) < 0)
goto unmap;
 
-   hashcpy(istate->sha1, (const unsigned char *)hdr + mmap_size - 20);
+   hashcpy(istate->sha1, (const unsigned char *)hdr + mmap_size - 
the_hash_algo->rawsz);
istate->version = ntohl(hdr->hdr_version);
istate->cache_nr = ntohl(hdr->hdr_entries);
istate->cache_alloc = alloc_nr(istate->cache_nr);
@@ -1831,7 +1831,7 @@ int do_read_index(struct index_state *istate, const char 
*path, int must_exist)
istate->timestamp.sec = st.st_mtime;
istate->timestamp.nsec = ST_MTIME_NSEC(st);
 
-   while (src_offset <= mmap_size - 20 - 8) {
+   while (src_offset <= mmap_size - the_hash_algo->rawsz - 8) {
/* After an array of active_nr index entries,
 * there can be arbitrary number of extended
 * sections, each of which is prefixed with
@@ -1957,11 +1957,11 @@ int unmerged_index(const struct index_state *istate)
 static unsigned char write_buffer[WRITE_BUFFER_SIZE];
 static unsigned long write_buffer_len;
 
-static int ce_write_flush(git_SHA_CTX *context, int fd)
+static int ce_write_flush(git_hash_ctx *context, int fd)
 {
unsigned int buffered = write_buffer_len;
if (buffered) {
-   git_SHA1_Update(context, write_buffer, buffered);
+   the_hash_algo->update_fn(context, write_buffer, buffered);
if (write_in_full(fd, write_buffer, buffered) < 0)
return -1;
write_buffer_len = 0;
@@ -1969,7 +1969,7 @@ static int ce_write_flush(git_SHA_CTX *context, int fd)
return 0;
 }
 
-static int ce_write(git_SHA_CTX *context, int fd, void *data, unsigned int len)
+static int ce_write(git_hash_ctx *context, int fd, void *data, unsigned int 
len)
 {
while (len) {
unsigned int buffered = write_buffer_len;
@@ -1991,7 +1991,7 @@ static int ce_write(git_SHA_CTX *context, int fd, void 
*data, unsigned int len)
return 0;
 }
 
-static int write_index_ext_header(git_SHA_CTX *context, int fd,
+static int write_index_ext_header(git_hash_ctx *context, int fd,
  unsigned int ext, unsigned int sz)
 {
ext = htonl(ext);
@@ -2000,26 +2000,26 @@ static int write_index_ext_header(git_SHA_CTX *context, 
int fd,
(ce_write(context, fd, , 4) < 0)) ? -1 : 0;
 }
 
-static int ce_flush(git_SHA_CTX *context, int fd, unsigned char *sha1)
+static int ce_flush(git_hash_ctx *context, int fd, unsigned char *hash)
 {
unsigned int left = write_buffer_len;
 
if (left) {
write_buffer_len = 0;
-   git_SHA1_Update(context, write_buffer, left);
+   the_hash_algo->update_fn(context, write_buffer, left);
}
 
-   /* Flush first if not enough space for SHA1 signature */
-   if (left + 20 > 

[PATCH v2 04/12] builtin/unpack-objects: switch uses of SHA-1 to the_hash_algo

[brian m. carlson]

Switch various uses of explicit calls to SHA-1 into references to
the_hash_algo to better abstract away the various uses of it.

Signed-off-by: brian m. carlson 
---
 builtin/unpack-objects.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 62ea264c46..813ca31979 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -21,7 +21,7 @@ static unsigned char buffer[4096];
 static unsigned int offset, len;
 static off_t consumed_bytes;
 static off_t max_input_size;
-static git_SHA_CTX ctx;
+static git_hash_ctx ctx;
 static struct fsck_options fsck_options = FSCK_OPTIONS_STRICT;
 
 /*
@@ -62,7 +62,7 @@ static void *fill(int min)
if (min > sizeof(buffer))
die("cannot fill %d bytes", min);
if (offset) {
-   git_SHA1_Update(, buffer, offset);
+   the_hash_algo->update_fn(, buffer, offset);
memmove(buffer, buffer + offset, len);
offset = 0;
}
@@ -345,8 +345,8 @@ static void unpack_delta_entry(enum object_type type, 
unsigned long delta_size,
struct object_id base_oid;
 
if (type == OBJ_REF_DELTA) {
-   hashcpy(base_oid.hash, fill(GIT_SHA1_RAWSZ));
-   use(GIT_SHA1_RAWSZ);
+   hashcpy(base_oid.hash, fill(the_hash_algo->rawsz));
+   use(the_hash_algo->rawsz);
delta_data = get_data(delta_size);
if (dry_run || !delta_data) {
free(delta_data);
@@ -564,15 +564,15 @@ int cmd_unpack_objects(int argc, const char **argv, const 
char *prefix)
/* We don't take any non-flag arguments now.. Maybe some day */
usage(unpack_usage);
}
-   git_SHA1_Init();
+   the_hash_algo->init_fn();
unpack_all();
-   git_SHA1_Update(, buffer, offset);
-   git_SHA1_Final(oid.hash, );
+   the_hash_algo->update_fn(, buffer, offset);
+   the_hash_algo->final_fn(oid.hash, );
if (strict)
write_rest();
-   if (hashcmp(fill(GIT_SHA1_RAWSZ), oid.hash))
+   if (hashcmp(fill(the_hash_algo->rawsz), oid.hash))
die("final sha1 did not match");
-   use(GIT_SHA1_RAWSZ);
+   use(the_hash_algo->rawsz);
 
/* Write the last part of the buffer to stdout */
while (len) {

[PATCH v2 01/12] hash: move SHA-1 macros to hash.h

[brian m. carlson]

Most of the other code dealing with SHA-1 and other hashes is located in
hash.h, which is in turn loaded by cache.h.  Move the SHA-1 macros to
hash.h as well, so we can use them in additional hash-related items in
the future.

Signed-off-by: brian m. carlson 
---
 cache.h | 25 -
 hash.h  | 25 +
 2 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/cache.h b/cache.h
index d8b975a571..bfde6f757a 100644
--- a/cache.h
+++ b/cache.h
@@ -16,31 +16,6 @@
 #include "sha1-array.h"
 #include "repository.h"
 
-#ifndef platform_SHA_CTX
-/*
- * platform's underlying implementation of SHA-1; could be OpenSSL,
- * blk_SHA, Apple CommonCrypto, etc...  Note that including
- * SHA1_HEADER may have already defined platform_SHA_CTX for our
- * own implementations like block-sha1 and ppc-sha1, so we list
- * the default for OpenSSL compatible SHA-1 implementations here.
- */
-#define platform_SHA_CTX   SHA_CTX
-#define platform_SHA1_Init SHA1_Init
-#define platform_SHA1_Update   SHA1_Update
-#define platform_SHA1_FinalSHA1_Final
-#endif
-
-#define git_SHA_CTXplatform_SHA_CTX
-#define git_SHA1_Init  platform_SHA1_Init
-#define git_SHA1_Updateplatform_SHA1_Update
-#define git_SHA1_Final platform_SHA1_Final
-
-#ifdef SHA1_MAX_BLOCK_SIZE
-#include "compat/sha1-chunked.h"
-#undef git_SHA1_Update
-#define git_SHA1_Updategit_SHA1_Update_Chunked
-#endif
-
 #include 
 typedef struct git_zstream {
z_stream z;
diff --git a/hash.h b/hash.h
index 7d7a864f5d..7122dea7b3 100644
--- a/hash.h
+++ b/hash.h
@@ -15,6 +15,31 @@
 #include "block-sha1/sha1.h"
 #endif
 
+#ifndef platform_SHA_CTX
+/*
+ * platform's underlying implementation of SHA-1; could be OpenSSL,
+ * blk_SHA, Apple CommonCrypto, etc...  Note that including
+ * SHA1_HEADER may have already defined platform_SHA_CTX for our
+ * own implementations like block-sha1 and ppc-sha1, so we list
+ * the default for OpenSSL compatible SHA-1 implementations here.
+ */
+#define platform_SHA_CTX   SHA_CTX
+#define platform_SHA1_Init SHA1_Init
+#define platform_SHA1_Update   SHA1_Update
+#define platform_SHA1_FinalSHA1_Final
+#endif
+
+#define git_SHA_CTXplatform_SHA_CTX
+#define git_SHA1_Init  platform_SHA1_Init
+#define git_SHA1_Updateplatform_SHA1_Update
+#define git_SHA1_Final platform_SHA1_Final
+
+#ifdef SHA1_MAX_BLOCK_SIZE
+#include "compat/sha1-chunked.h"
+#undef git_SHA1_Update
+#define git_SHA1_Updategit_SHA1_Update_Chunked
+#endif
+
 /*
  * Note that these constants are suitable for indexing the hash_algos array and
  * comparing against each other, but are otherwise arbitrary, so they should 
not

[PATCH v2 12/12] bulk-checkin: abstract SHA-1 usage

[brian m. carlson]

Convert uses of the direct SHA-1 functions to use the_hash_algo instead.

Signed-off-by: brian m. carlson 
---
 bulk-checkin.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/bulk-checkin.c b/bulk-checkin.c
index 5f79ed6ea3..8bcd1c8665 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -93,7 +93,7 @@ static int already_written(struct bulk_checkin_state *state, 
unsigned char sha1[
  * with a new pack.
  */
 static int stream_to_pack(struct bulk_checkin_state *state,
- git_SHA_CTX *ctx, off_t *already_hashed_to,
+ git_hash_ctx *ctx, off_t *already_hashed_to,
  int fd, size_t size, enum object_type type,
  const char *path, unsigned flags)
 {
@@ -127,7 +127,7 @@ static int stream_to_pack(struct bulk_checkin_state *state,
if (rsize < hsize)
hsize = rsize;
if (hsize)
-   git_SHA1_Update(ctx, ibuf, hsize);
+   the_hash_algo->update_fn(ctx, ibuf, 
hsize);
*already_hashed_to = offset;
}
s.next_in = ibuf;
@@ -192,7 +192,7 @@ static int deflate_to_pack(struct bulk_checkin_state *state,
   unsigned flags)
 {
off_t seekback, already_hashed_to;
-   git_SHA_CTX ctx;
+   git_hash_ctx ctx;
unsigned char obuf[16384];
unsigned header_len;
struct hashfile_checkpoint checkpoint;
@@ -204,8 +204,8 @@ static int deflate_to_pack(struct bulk_checkin_state *state,
 
header_len = xsnprintf((char *)obuf, sizeof(obuf), "%s %" PRIuMAX,
   typename(type), (uintmax_t)size) + 1;
-   git_SHA1_Init();
-   git_SHA1_Update(, obuf, header_len);
+   the_hash_algo->init_fn();
+   the_hash_algo->update_fn(, obuf, header_len);
 
/* Note: idx is non-NULL when we are writing */
if ((flags & HASH_WRITE_OBJECT) != 0)
@@ -236,7 +236,7 @@ static int deflate_to_pack(struct bulk_checkin_state *state,
if (lseek(fd, seekback, SEEK_SET) == (off_t) -1)
return error("cannot seek back");
}
-   git_SHA1_Final(result_sha1, );
+   the_hash_algo->final_fn(result_sha1, );
if (!idx)
return 0;
 

[PATCH v2 08/12] pack-write: switch various SHA-1 values to abstract forms

[brian m. carlson]

Convert various uses of hardcoded 20- and 40-based numbers to use
the_hash_algo, along with direct calls to SHA-1.  Adjust the names of
variables to refer to "hash" instead of "sha1".

Signed-off-by: brian m. carlson 
---
 pack-write.c | 49 +
 1 file changed, 25 insertions(+), 24 deletions(-)

diff --git a/pack-write.c b/pack-write.c
index fea6284192..5317678c29 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -122,7 +122,7 @@ const char *write_idx_file(const char *index_name, struct 
pack_idx_entry **objec
uint32_t offset = htonl(obj->offset);
sha1write(f, , 4);
}
-   sha1write(f, obj->oid.hash, 20);
+   sha1write(f, obj->oid.hash, the_hash_algo->rawsz);
if ((opts->flags & WRITE_IDX_STRICT) &&
(i && !oidcmp([-2]->oid, >oid)))
die("The same object %s appears twice in the pack",
@@ -169,7 +169,7 @@ const char *write_idx_file(const char *index_name, struct 
pack_idx_entry **objec
}
}
 
-   sha1write(f, sha1, 20);
+   sha1write(f, sha1, the_hash_algo->rawsz);
sha1close(f, NULL, ((opts->flags & WRITE_IDX_VERIFY)
? CSUM_CLOSE : CSUM_FSYNC));
return index_name;
@@ -203,20 +203,20 @@ off_t write_pack_header(struct sha1file *f, uint32_t 
nr_entries)
  * interested in the resulting SHA1 of pack data above partial_pack_offset.
  */
 void fixup_pack_header_footer(int pack_fd,
-unsigned char *new_pack_sha1,
+unsigned char *new_pack_hash,
 const char *pack_name,
 uint32_t object_count,
-unsigned char *partial_pack_sha1,
+unsigned char *partial_pack_hash,
 off_t partial_pack_offset)
 {
int aligned_sz, buf_sz = 8 * 1024;
-   git_SHA_CTX old_sha1_ctx, new_sha1_ctx;
+   git_hash_ctx old_hash_ctx, new_hash_ctx;
struct pack_header hdr;
char *buf;
ssize_t read_result;
 
-   git_SHA1_Init(_sha1_ctx);
-   git_SHA1_Init(_sha1_ctx);
+   the_hash_algo->init_fn(_hash_ctx);
+   the_hash_algo->init_fn(_hash_ctx);
 
if (lseek(pack_fd, 0, SEEK_SET) != 0)
die_errno("Failed seeking to start of '%s'", pack_name);
@@ -228,9 +228,9 @@ void fixup_pack_header_footer(int pack_fd,
  pack_name);
if (lseek(pack_fd, 0, SEEK_SET) != 0)
die_errno("Failed seeking to start of '%s'", pack_name);
-   git_SHA1_Update(_sha1_ctx, , sizeof(hdr));
+   the_hash_algo->update_fn(_hash_ctx, , sizeof(hdr));
hdr.hdr_entries = htonl(object_count);
-   git_SHA1_Update(_sha1_ctx, , sizeof(hdr));
+   the_hash_algo->update_fn(_hash_ctx, , sizeof(hdr));
write_or_die(pack_fd, , sizeof(hdr));
partial_pack_offset -= sizeof(hdr);
 
@@ -238,28 +238,28 @@ void fixup_pack_header_footer(int pack_fd,
aligned_sz = buf_sz - sizeof(hdr);
for (;;) {
ssize_t m, n;
-   m = (partial_pack_sha1 && partial_pack_offset < aligned_sz) ?
+   m = (partial_pack_hash && partial_pack_offset < aligned_sz) ?
partial_pack_offset : aligned_sz;
n = xread(pack_fd, buf, m);
if (!n)
break;
if (n < 0)
die_errno("Failed to checksum '%s'", pack_name);
-   git_SHA1_Update(_sha1_ctx, buf, n);
+   the_hash_algo->update_fn(_hash_ctx, buf, n);
 
aligned_sz -= n;
if (!aligned_sz)
aligned_sz = buf_sz;
 
-   if (!partial_pack_sha1)
+   if (!partial_pack_hash)
continue;
 
-   git_SHA1_Update(_sha1_ctx, buf, n);
+   the_hash_algo->update_fn(_hash_ctx, buf, n);
partial_pack_offset -= n;
if (partial_pack_offset == 0) {
-   unsigned char sha1[20];
-   git_SHA1_Final(sha1, _sha1_ctx);
-   if (hashcmp(sha1, partial_pack_sha1) != 0)
+   unsigned char hash[GIT_MAX_RAWSZ];
+   the_hash_algo->final_fn(hash, _hash_ctx);
+   if (hashcmp(hash, partial_pack_hash) != 0)
die("Unexpected checksum for %s "
"(disk corruption?)", pack_name);
/*
@@ -267,23 +267,24 @@ void fixup_pack_header_footer(int pack_fd,
 * pack, which also means making partial_pack_offset
 * big enough not to matter anymore.
 */
-   git_SHA1_Init(_sha1_ctx);
+   

[PATCH v2 11/12] csum-file: abstract uses of SHA-1

[brian m. carlson]

Convert several direct uses of SHA-1 to use the_hash_algo instead.
Convert one use of the constant 20 as well.

Signed-off-by: brian m. carlson 
---
 csum-file.c | 10 +-
 csum-file.h |  4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/csum-file.c b/csum-file.c
index e4ad6337dc..5eda7fb6af 100644
--- a/csum-file.c
+++ b/csum-file.c
@@ -47,7 +47,7 @@ void hashflush(struct hashfile *f)
unsigned offset = f->offset;
 
if (offset) {
-   git_SHA1_Update(>ctx, f->buffer, offset);
+   the_hash_algo->update_fn(>ctx, f->buffer, offset);
flush(f, f->buffer, offset);
f->offset = 0;
}
@@ -58,12 +58,12 @@ int hashclose(struct hashfile *f, unsigned char *result, 
unsigned int flags)
int fd;
 
hashflush(f);
-   git_SHA1_Final(f->buffer, >ctx);
+   the_hash_algo->final_fn(f->buffer, >ctx);
if (result)
hashcpy(result, f->buffer);
if (flags & (CSUM_CLOSE | CSUM_FSYNC)) {
/* write checksum and close fd */
-   flush(f, f->buffer, 20);
+   flush(f, f->buffer, the_hash_algo->rawsz);
if (flags & CSUM_FSYNC)
fsync_or_die(f->fd, f->name);
if (close(f->fd))
@@ -110,7 +110,7 @@ void hashwrite(struct hashfile *f, const void *buf, 
unsigned int count)
buf = (char *) buf + nr;
left -= nr;
if (!left) {
-   git_SHA1_Update(>ctx, data, offset);
+   the_hash_algo->update_fn(>ctx, data, offset);
flush(f, data, offset);
offset = 0;
}
@@ -149,7 +149,7 @@ struct hashfile *hashfd_throughput(int fd, const char 
*name, struct progress *tp
f->tp = tp;
f->name = name;
f->do_crc = 0;
-   git_SHA1_Init(>ctx);
+   the_hash_algo->init_fn(>ctx);
return f;
 }
 
diff --git a/csum-file.h b/csum-file.h
index ceb3e5712d..992e5c0141 100644
--- a/csum-file.h
+++ b/csum-file.h
@@ -8,7 +8,7 @@ struct hashfile {
int fd;
int check_fd;
unsigned int offset;
-   git_SHA_CTX ctx;
+   git_hash_ctx ctx;
off_t total;
struct progress *tp;
const char *name;
@@ -20,7 +20,7 @@ struct hashfile {
 /* Checkpoint */
 struct hashfile_checkpoint {
off_t offset;
-   git_SHA_CTX ctx;
+   git_hash_ctx ctx;
 };
 
 extern void hashfile_checkpoint(struct hashfile *, struct hashfile_checkpoint 
*);

[PATCH v2 06/12] fast-import: switch various uses of SHA-1 to the_hash_algo

[brian m. carlson]

Switch various uses of explicit calls to SHA-1 to use the_hash_algo.
Convert various uses of 20 and the GIT_SHA1 constants as well.

Signed-off-by: brian m. carlson 
---
 fast-import.c | 42 ++
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index b70ac025e0..f20f30ab62 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1092,15 +1092,15 @@ static int store_object(
unsigned char hdr[96];
struct object_id oid;
unsigned long hdrlen, deltalen;
-   git_SHA_CTX c;
+   git_hash_ctx c;
git_zstream s;
 
hdrlen = xsnprintf((char *)hdr, sizeof(hdr), "%s %lu",
   typename(type), (unsigned long)dat->len) + 1;
-   git_SHA1_Init();
-   git_SHA1_Update(, hdr, hdrlen);
-   git_SHA1_Update(, dat->buf, dat->len);
-   git_SHA1_Final(oid.hash, );
+   the_hash_algo->init_fn();
+   the_hash_algo->update_fn(, hdr, hdrlen);
+   the_hash_algo->update_fn(, dat->buf, dat->len);
+   the_hash_algo->final_fn(oid.hash, );
if (oidout)
oidcpy(oidout, );
 
@@ -1118,11 +1118,13 @@ static int store_object(
return 1;
}
 
-   if (last && last->data.buf && last->depth < max_depth && dat->len > 20) 
{
+   if (last && last->data.buf && last->depth < max_depth
+   && dat->len > the_hash_algo->rawsz) {
+
delta_count_attempts_by_type[type]++;
delta = diff_delta(last->data.buf, last->data.len,
dat->buf, dat->len,
-   , dat->len - 20);
+   , dat->len - the_hash_algo->rawsz);
} else
delta = NULL;
 
@@ -1231,7 +1233,7 @@ static void stream_blob(uintmax_t len, struct object_id 
*oidout, uintmax_t mark)
struct object_id oid;
unsigned long hdrlen;
off_t offset;
-   git_SHA_CTX c;
+   git_hash_ctx c;
git_zstream s;
struct sha1file_checkpoint checkpoint;
int status = Z_OK;
@@ -1246,8 +1248,8 @@ static void stream_blob(uintmax_t len, struct object_id 
*oidout, uintmax_t mark)
 
hdrlen = xsnprintf((char *)out_buf, out_sz, "blob %" PRIuMAX, len) + 1;
 
-   git_SHA1_Init();
-   git_SHA1_Update(, out_buf, hdrlen);
+   the_hash_algo->init_fn();
+   the_hash_algo->update_fn(, out_buf, hdrlen);
 
crc32_begin(pack_file);
 
@@ -1265,7 +1267,7 @@ static void stream_blob(uintmax_t len, struct object_id 
*oidout, uintmax_t mark)
if (!n && feof(stdin))
die("EOF in data (%" PRIuMAX " bytes 
remaining)", len);
 
-   git_SHA1_Update(, in_buf, n);
+   the_hash_algo->update_fn(, in_buf, n);
s.next_in = in_buf;
s.avail_in = n;
len -= n;
@@ -1291,7 +1293,7 @@ static void stream_blob(uintmax_t len, struct object_id 
*oidout, uintmax_t mark)
}
}
git_deflate_end();
-   git_SHA1_Final(oid.hash, );
+   the_hash_algo->final_fn(oid.hash, );
 
if (oidout)
oidcpy(oidout, );
@@ -1350,11 +1352,11 @@ static void *gfi_unpack_entry(
 {
enum object_type type;
struct packed_git *p = all_packs[oe->pack_id];
-   if (p == pack_data && p->pack_size < (pack_size + 20)) {
+   if (p == pack_data && p->pack_size < (pack_size + 
the_hash_algo->rawsz)) {
/* The object is stored in the packfile we are writing to
 * and we have modified it since the last time we scanned
 * back to read a previously written object.  If an old
-* window covered [p->pack_size, p->pack_size + 20) its
+* window covered [p->pack_size, p->pack_size + rawsz) its
 * data is stale and is not valid.  Closing all windows
 * and updating the packfile length ensures we can read
 * the newly written data.
@@ -1362,13 +1364,13 @@ static void *gfi_unpack_entry(
close_pack_windows(p);
sha1flush(pack_file);
 
-   /* We have to offer 20 bytes additional on the end of
+   /* We have to offer rawsz bytes additional on the end of
 * the packfile as the core unpacker code assumes the
 * footer is present at the file end and must promise
-* at least 20 bytes within any window it maps.  But
+* at least rawsz bytes within any window it maps.  But
 * we don't actually create the footer here.
 */
-   p->pack_size = pack_size + 20;
+   p->pack_size = pack_size + the_hash_algo->rawsz;
}
return unpack_entry(p, oe->idx.offset, , sizep);
 }
@@ -2204,7 +2206,7 @@ static void 

[PATCH v2 02/12] hash: create union for hash context allocation

[brian m. carlson]

In various parts of our code, we want to allocate a structure
representing the internal state of a hash algorithm.  The original
implementation of the hash algorithm abstraction assumed we would do
that using heap allocations, and added a context size element to struct
git_hash_algo.  However, most of the existing code uses stack
allocations and conversion would needlessly complicate various parts of
the code.  Add a union for the purpose of allocating hash contexts on
the stack and a typedef for ease of use.  Use this union for defining
the init, update, and final functions to avoid casts.  Remove the ctxsz
element for struct git_hash_algo, which is no longer very useful.

This does mean that stack allocations will grow slightly as additional
hash functions are added, but this should not be a significant problem,
since we don't allocate many hash contexts.  The improved usability and
benefits from avoiding dynamic allocation outweigh this small downside.

Signed-off-by: brian m. carlson 
---
 hash.h  | 15 +--
 sha1_file.c | 20 +---
 2 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/hash.h b/hash.h
index 7122dea7b3..eb30f59be3 100644
--- a/hash.h
+++ b/hash.h
@@ -55,9 +55,15 @@
 /* Number of algorithms supported (including unknown). */
 #define GIT_HASH_NALGOS (GIT_HASH_SHA1 + 1)
 
-typedef void (*git_hash_init_fn)(void *ctx);
-typedef void (*git_hash_update_fn)(void *ctx, const void *in, size_t len);
-typedef void (*git_hash_final_fn)(unsigned char *hash, void *ctx);
+/* A suitably aligned type for stack allocations of hash contexts. */
+union git_hash_ctx {
+   git_SHA_CTX sha1;
+};
+typedef union git_hash_ctx git_hash_ctx;
+
+typedef void (*git_hash_init_fn)(git_hash_ctx *ctx);
+typedef void (*git_hash_update_fn)(git_hash_ctx *ctx, const void *in, size_t 
len);
+typedef void (*git_hash_final_fn)(unsigned char *hash, git_hash_ctx *ctx);
 
 struct git_hash_algo {
/*
@@ -69,9 +75,6 @@ struct git_hash_algo {
/* A four-byte version identifier, used in pack indices. */
uint32_t format_id;
 
-   /* The size of a hash context (e.g. git_SHA_CTX). */
-   size_t ctxsz;
-
/* The length of the hash in binary. */
size_t rawsz;
 
diff --git a/sha1_file.c b/sha1_file.c
index 3da70ac650..ec6ecea170 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -39,32 +39,32 @@ const struct object_id empty_blob_oid = {
EMPTY_BLOB_SHA1_BIN_LITERAL
 };
 
-static void git_hash_sha1_init(void *ctx)
+static void git_hash_sha1_init(git_hash_ctx *ctx)
 {
-   git_SHA1_Init((git_SHA_CTX *)ctx);
+   git_SHA1_Init(>sha1);
 }
 
-static void git_hash_sha1_update(void *ctx, const void *data, size_t len)
+static void git_hash_sha1_update(git_hash_ctx *ctx, const void *data, size_t 
len)
 {
-   git_SHA1_Update((git_SHA_CTX *)ctx, data, len);
+   git_SHA1_Update(>sha1, data, len);
 }
 
-static void git_hash_sha1_final(unsigned char *hash, void *ctx)
+static void git_hash_sha1_final(unsigned char *hash, git_hash_ctx *ctx)
 {
-   git_SHA1_Final(hash, (git_SHA_CTX *)ctx);
+   git_SHA1_Final(hash, >sha1);
 }
 
-static void git_hash_unknown_init(void *ctx)
+static void git_hash_unknown_init(git_hash_ctx *ctx)
 {
die("trying to init unknown hash");
 }
 
-static void git_hash_unknown_update(void *ctx, const void *data, size_t len)
+static void git_hash_unknown_update(git_hash_ctx *ctx, const void *data, 
size_t len)
 {
die("trying to update unknown hash");
 }
 
-static void git_hash_unknown_final(unsigned char *hash, void *ctx)
+static void git_hash_unknown_final(unsigned char *hash, git_hash_ctx *ctx)
 {
die("trying to finalize unknown hash");
 }
@@ -75,7 +75,6 @@ const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {
0x,
0,
0,
-   0,
git_hash_unknown_init,
git_hash_unknown_update,
git_hash_unknown_final,
@@ -86,7 +85,6 @@ const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {
"sha-1",
/* "sha1", big-endian */
0x73686131,
-   sizeof(git_SHA_CTX),
GIT_SHA1_RAWSZ,
GIT_SHA1_HEXSZ,
git_hash_sha1_init,

[PATCH v2 00/12] object_id part 11 (the_hash_algo)

[brian m. carlson]

This series includes various changes to adopt the use of the_hash_algo
for abstracting hash algorithms away.

Changes from v1:
* Fix comments referring to SHA-1.
* Switch hash function wrappers to take git_hash_ctx *.
* Use a const int variable for a constant value.
* Wrap an overly long line.

brian m. carlson (12):
  hash: move SHA-1 macros to hash.h
  hash: create union for hash context allocation
  builtin/index-pack: improve hash function abstraction
  builtin/unpack-objects: switch uses of SHA-1 to the_hash_algo
  sha1_file: switch uses of SHA-1 to the_hash_algo
  fast-import: switch various uses of SHA-1 to the_hash_algo
  pack-check: convert various uses of SHA-1 to abstract forms
  pack-write: switch various SHA-1 values to abstract forms
  read-cache: abstract away uses of SHA-1
  csum-file: rename sha1file to hashfile
  csum-file: abstract uses of SHA-1
  bulk-checkin: abstract SHA-1 usage

 builtin/index-pack.c | 108 +++
 builtin/pack-objects.c   |  52 +++
 builtin/unpack-objects.c |  18 
 bulk-checkin.c   |  28 ++--
 cache.h  |  25 ---
 csum-file.c  |  46 ++--
 csum-file.h  |  38 -
 fast-import.c|  70 +++---
 hash.h   |  40 +++---
 pack-bitmap-write.c  |  30 ++---
 pack-check.c |  32 +++---
 pack-write.c |  77 -
 pack.h   |   4 +-
 read-cache.c |  58 -
 sha1_file.c  |  72 +++
 15 files changed, 351 insertions(+), 347 deletions(-)

[PATCH v2 07/12] pack-check: convert various uses of SHA-1 to abstract forms

[brian m. carlson]

Convert various explicit calls to use SHA-1 functions and constants to
references to the_hash_algo.  Make several strings more generic with
respect to the hash algorithm used.

Signed-off-by: brian m. carlson 
---
 pack-check.c | 32 
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/pack-check.c b/pack-check.c
index 073c1fbd46..403a572567 100644
--- a/pack-check.c
+++ b/pack-check.c
@@ -41,7 +41,7 @@ int check_pack_crc(struct packed_git *p, struct pack_window 
**w_curs,
} while (len);
 
index_crc = p->index_data;
-   index_crc += 2 + 256 + p->num_objects * (20/4) + nr;
+   index_crc += 2 + 256 + p->num_objects * (the_hash_algo->rawsz/4) + nr;
 
return data_crc != ntohl(*index_crc);
 }
@@ -54,7 +54,7 @@ static int verify_packfile(struct packed_git *p,
 {
off_t index_size = p->index_size;
const unsigned char *index_base = p->index_data;
-   git_SHA_CTX ctx;
+   git_hash_ctx ctx;
unsigned char hash[GIT_MAX_RAWSZ], *pack_sig;
off_t offset = 0, pack_sig_ofs = 0;
uint32_t nr_objects, i;
@@ -64,24 +64,24 @@ static int verify_packfile(struct packed_git *p,
if (!is_pack_valid(p))
return error("packfile %s cannot be accessed", p->pack_name);
 
-   git_SHA1_Init();
+   the_hash_algo->init_fn();
do {
unsigned long remaining;
unsigned char *in = use_pack(p, w_curs, offset, );
offset += remaining;
if (!pack_sig_ofs)
-   pack_sig_ofs = p->pack_size - 20;
+   pack_sig_ofs = p->pack_size - the_hash_algo->rawsz;
if (offset > pack_sig_ofs)
remaining -= (unsigned int)(offset - pack_sig_ofs);
-   git_SHA1_Update(, in, remaining);
+   the_hash_algo->update_fn(, in, remaining);
} while (offset < pack_sig_ofs);
-   git_SHA1_Final(hash, );
+   the_hash_algo->final_fn(hash, );
pack_sig = use_pack(p, w_curs, pack_sig_ofs, NULL);
if (hashcmp(hash, pack_sig))
-   err = error("%s SHA1 checksum mismatch",
+   err = error("%s pack checksum mismatch",
p->pack_name);
-   if (hashcmp(index_base + index_size - 40, pack_sig))
-   err = error("%s SHA1 does not match its index",
+   if (hashcmp(index_base + index_size - the_hash_algo->hexsz, pack_sig))
+   err = error("%s pack checksum does not match its index",
p->pack_name);
unuse_pack(w_curs);
 
@@ -165,8 +165,8 @@ int verify_pack_index(struct packed_git *p)
 {
off_t index_size;
const unsigned char *index_base;
-   git_SHA_CTX ctx;
-   unsigned char sha1[20];
+   git_hash_ctx ctx;
+   unsigned char hash[GIT_MAX_RAWSZ];
int err = 0;
 
if (open_pack_index(p))
@@ -175,11 +175,11 @@ int verify_pack_index(struct packed_git *p)
index_base = p->index_data;
 
/* Verify SHA1 sum of the index file */
-   git_SHA1_Init();
-   git_SHA1_Update(, index_base, (unsigned int)(index_size - 20));
-   git_SHA1_Final(sha1, );
-   if (hashcmp(sha1, index_base + index_size - 20))
-   err = error("Packfile index for %s SHA1 mismatch",
+   the_hash_algo->init_fn();
+   the_hash_algo->update_fn(, index_base, (unsigned int)(index_size - 
the_hash_algo->rawsz));
+   the_hash_algo->final_fn(hash, );
+   if (hashcmp(hash, index_base + index_size - the_hash_algo->rawsz))
+   err = error("Packfile index for %s hash mismatch",
p->pack_name);
return err;
 }

[PATCH v2 03/12] builtin/index-pack: improve hash function abstraction

[brian m. carlson]

Convert several uses of unsigned char [20] to struct object_id and
convert various hard-coded constants and uses of SHA-1 functions to use
the_hash_algo.

Signed-off-by: brian m. carlson 
---
 builtin/index-pack.c | 90 ++--
 1 file changed, 45 insertions(+), 45 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 4c51aec81f..40c000aca8 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -91,7 +91,7 @@ static unsigned int input_offset, input_len;
 static off_t consumed_bytes;
 static off_t max_input_size;
 static unsigned deepest_delta;
-static git_SHA_CTX input_ctx;
+static git_hash_ctx input_ctx;
 static uint32_t input_crc32;
 static int input_fd, output_fd;
 static const char *curr_pack;
@@ -253,7 +253,7 @@ static void flush(void)
if (input_offset) {
if (output_fd >= 0)
write_or_die(output_fd, input_buffer, input_offset);
-   git_SHA1_Update(_ctx, input_buffer, input_offset);
+   the_hash_algo->update_fn(_ctx, input_buffer, 
input_offset);
memmove(input_buffer, input_buffer + input_offset, input_len);
input_offset = 0;
}
@@ -326,7 +326,7 @@ static const char *open_pack_file(const char *pack_name)
output_fd = -1;
nothread_data.pack_fd = input_fd;
}
-   git_SHA1_Init(_ctx);
+   the_hash_algo->init_fn(_ctx);
return pack_name;
 }
 
@@ -437,22 +437,22 @@ static int is_delta_type(enum object_type type)
 }
 
 static void *unpack_entry_data(off_t offset, unsigned long size,
-  enum object_type type, unsigned char *sha1)
+  enum object_type type, struct object_id *oid)
 {
static char fixed_buf[8192];
int status;
git_zstream stream;
void *buf;
-   git_SHA_CTX c;
+   git_hash_ctx c;
char hdr[32];
int hdrlen;
 
if (!is_delta_type(type)) {
hdrlen = xsnprintf(hdr, sizeof(hdr), "%s %lu", typename(type), 
size) + 1;
-   git_SHA1_Init();
-   git_SHA1_Update(, hdr, hdrlen);
+   the_hash_algo->init_fn();
+   the_hash_algo->update_fn(, hdr, hdrlen);
} else
-   sha1 = NULL;
+   oid = NULL;
if (type == OBJ_BLOB && size > big_file_threshold)
buf = fixed_buf;
else
@@ -469,8 +469,8 @@ static void *unpack_entry_data(off_t offset, unsigned long 
size,
stream.avail_in = input_len;
status = git_inflate(, 0);
use(input_len - stream.avail_in);
-   if (sha1)
-   git_SHA1_Update(, last_out, stream.next_out - 
last_out);
+   if (oid)
+   the_hash_algo->update_fn(, last_out, stream.next_out 
- last_out);
if (buf == fixed_buf) {
stream.next_out = buf;
stream.avail_out = sizeof(fixed_buf);
@@ -479,15 +479,15 @@ static void *unpack_entry_data(off_t offset, unsigned 
long size,
if (stream.total_out != size || status != Z_STREAM_END)
bad_object(offset, _("inflate returned %d"), status);
git_inflate_end();
-   if (sha1)
-   git_SHA1_Final(sha1, );
+   if (oid)
+   the_hash_algo->final_fn(oid->hash, );
return buf == fixed_buf ? NULL : buf;
 }
 
 static void *unpack_raw_entry(struct object_entry *obj,
  off_t *ofs_offset,
- unsigned char *ref_sha1,
- unsigned char *sha1)
+ struct object_id *ref_oid,
+ struct object_id *oid)
 {
unsigned char *p;
unsigned long size, c;
@@ -515,8 +515,8 @@ static void *unpack_raw_entry(struct object_entry *obj,
 
switch (obj->type) {
case OBJ_REF_DELTA:
-   hashcpy(ref_sha1, fill(20));
-   use(20);
+   hashcpy(ref_oid->hash, fill(the_hash_algo->rawsz));
+   use(the_hash_algo->rawsz);
break;
case OBJ_OFS_DELTA:
p = fill(1);
@@ -546,7 +546,7 @@ static void *unpack_raw_entry(struct object_entry *obj,
}
obj->hdr_size = consumed_bytes - obj->idx.offset;
 
-   data = unpack_entry_data(obj->idx.offset, obj->size, obj->type, sha1);
+   data = unpack_entry_data(obj->idx.offset, obj->size, obj->type, oid);
obj->idx.crc32 = input_crc32;
return data;
 }
@@ -1119,11 +1119,11 @@ static void *threaded_second_pass(void *data)
  * - calculate SHA1 of all non-delta objects;
  * - remember base (SHA1 or offset) for all deltas.
  */
-static void parse_pack_objects(unsigned char *sha1)
+static void parse_pack_objects(unsigned char *hash)
 {
int i, nr_delays = 0;

Re: [RFC PATCH 0/2] alternate hash test

[brian m. carlson]

On Tue, Jan 30, 2018 at 05:04:52PM -0800, Stefan Beller wrote:
> Thanks for writing this, I chose a slightly different approach at
> https://public-inbox.org/git/20170728171817.21458-2-sbel...@google.com/
> which might be quicker for local testing.

I'm actually going to rework the patches to be much more similar to
that.  I've been convinced that's an easier approach to the series, and
I like the simplicity.

My goal is not only to identify failing tests, but to exercise the hash
algorithm logic so that we find any hidden dependencies outside of that
code.  For example, I noticed some of our Perl scripts have hard-coded
empty-tree object IDs in them, which obviously will need to be fixed.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

What's cooking in git.git (Jan 2018, #04; Wed, 31)

[Junio C Hamano]

Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.  The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.

Many new topics appeared and are not yet marked with "Will do what"
labels.  Except for some large ones, I think most are already in
good shape, but I'd want to double check by re-reading them.

I am migrating my build and integration environment to a different
machine; if you notice anything out of ordinary, please let me know
before I decomission and reimage my usual environment ;-)

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[New Topics]

* ab/fetch-prune (2018-01-24) 11 commits
 - fetch: add a --fetch-prune option and fetch.pruneTags config
 - fetch tests: add scaffolding for the new fetch.pruneTags
 - git-fetch & config doc: link to the new PRUNING section
 - git remote doc: correct dangerous lies about what prune does
 - git fetch doc: add a new section to explain the ins & outs of pruning
 - fetch tests: test --prune and refspec interaction
 - fetch tests: add a tag to be deleted to the pruning tests
 - fetch tests: re-arrange arguments for future readability
 - fetch tests: refactor in preparation for testing tag pruning
 - fetch: stop accessing "remote" variable indirectly
 - fetch: don't redundantly NULL something calloc() gave us

 Clarify how configured fetch refspecs interact with the "--prune"
 option of "git fetch", and also add a handy short-hand for getting
 rid of stale tags that are locally held.


* gs/retire-mru (2018-01-24) 1 commit
 - mru: Replace mru.[ch] with list.h implementation
 (this branch uses ot/mru-on-list.)

 Retire mru API as it does not give enough abstraction over
 underlying list API to be worth it.

 Will merge to 'next'.


* jc/mailinfo-cleanup-fix (2018-01-24) 1 commit
 - mailinfo: avoid segfault when can't open files

 Corner case bugfix.

 Will merge to 'next'.


* po/clang-format-functype-weight (2018-01-24) 1 commit
 - clang-format: adjust penalty for return type line break

 Prevent "clang-format" from breaking line after function return type.

 Will merge to 'next'.


* po/http-push-error-message (2018-01-24) 1 commit
 - http-push: improve error log

 Debugging aid.

 Will merge to 'next'.


* po/object-id (2018-01-30) 12 commits
 - sha1_file: rename hash_sha1_file_literally
 - sha1_file: convert write_loose_object to object_id
 - sha1_file: convert force_object_loose to object_id
 - sha1_file: convert write_sha1_file to object_id
 - notes: convert write_notes_tree to object_id
 - notes: convert combine_notes_* to object_id
 - commit: convert commit_tree* to object_id
 - match-trees: convert splice_tree to object_id
 - cache: clear whole hash buffer with oidclr
 - sha1_file: convert hash_sha1_file to object_id
 - dir: convert struct sha1_stat to use object_id
 - sha1_file: convert pretend_sha1_file to object_id

 Conversion from uchar[20] to struct object_id continues.


* sg/travis-linux32-sanity (2018-01-30) 5 commits
 - travis-ci: don't fail if user already exists on 32 bit Linux build job
 - travis-ci: don't run the test suite as root in the 32 bit Linux build
 - travis-ci: don't repeat the path of the cache directory
 - travis-ci: use 'set -e' in the 32 bit Linux build job
 - travis-ci: use 'set -x' for the commands under 'su' in the 32 bit Linux build

 Will merge to 'next'.


* jk/daemon-fixes (2018-01-25) 6 commits
 - daemon: fix length computation in newline stripping
 - t/lib-git-daemon: add network-protocol helpers
 - daemon: handle NULs in extended attribute string
 - daemon: fix off-by-one in logging extended attributes
 - t/lib-git-daemon: record daemon log
 - t5570: use ls-remote instead of clone for interp tests


* jt/long-running-process-doc (2018-01-25) 1 commit
 - Docs: split out long-running subprocess handshake


* nd/format-patch-stat-width (2018-01-25) 2 commits
 - format-patch: reduce patch diffstat width to 72
 - format-patch: keep cover-letter diffstat wrapped in 72 columns


* nd/list-merge-strategy (2018-01-26) 1 commit
 - completion: fix completing merge strategies on non-C locales


* sb/pull-rebase-submodule (2018-01-25) 1 commit
 - builtin/pull: respect verbosity settings in submodules


* sg/test-i18ngrep (2018-01-26) 10 commits
 - t: make 'test_i18ngrep' more informative on failure
 - t: make sure that 'test_i18ngrep' got enough parameters
 - t: forbid piping into 'test_i18ngrep'
 - t: move 'test_i18ncmp' and 'test_i18ngrep' to 'test-lib-functions.sh'
 - t5536: let 'test_i18ngrep' read the file without redirection
 - t5510: consolidate 'grep' and 'test_i18ngrep' patterns
 - t4001: don't run 'git status' upstream of a pipe
 - t6022: don't run 'git merge' upstream of a pipe
 - t5812: add 

Re: [PATCH v2 01/41] parse-options: support --git-completion-helper

[Duy Nguyen]

On Thu, Feb 1, 2018 at 4:04 AM, Eric Sunshine  wrote:
> On Wed, Jan 31, 2018 at 6:05 AM, Nguyễn Thái Ngọc Duy  
> wrote:
>> This option is designed to be used by git-completion.bash. For many
>> simple cases, what we do in there is usually
>>
>> __gitcomp "lots of completion options"
>>
>> which has to be manually updated when a new user-visible option is
>> added. With support from parse-options, we can write
>>
>> __gitcomp "$(git command --git-completion-helper)"
>>
>> and get that list directly from the parser for free. Dangerous/Unpopular
>> options could be hidden with the new "NOCOMPLETE" flag.
>
> I wonder if this option should be named DANGEROUS rather than
> NOCOMPLETE to better reflect its intention.

It's not only for dangerous options (I forgot to mention this in the
commit message, I will in v3). The --continue|--abort|--skip should
only show up when you are in a middle of rebase/am/cherry-pick.
git-completion.bash handles this case separately and only put them in
the completion list  when appropriate. --git-completion-helper must
not include these or the trick done by git-completion.bash becomes
useless.

> The reason I ask is that
> it is easy to imagine git-completion.bash some day growing a new
> configuration option to allow people to complete these "dangerous"
> options, as well, rather than us imposing, with no escape hatch, our
> idea of what should and should not complete.
>
> It's not uncommon for "bug reports" to be sent to the list stating
> that such-and-such option (say, --force) does not autocomplete. Our
> stock answer is "oh, that's a dangerous option, so you'll have to type
> it manually". If git-completion.bash gains new configuration to allow
> dangerous options, then our answer can become "oh, that's a dangerous
> option, if you really want it to complete, then enable
> GIT_COMPLETION_DANGEROUS" (or whatever).

Interesting. So we now have two classes of "no complete". One can't be
configurable (--continue|--abort|--skip) and one can. I'll use two
separate flags for these, though I'm not adding the configuration
option right now. It's easy to do and can way until someone actually
asks for it.
-- 
Duy

Re: [PATCH] gitignore.txt: elaborate shell glob syntax

[Junio C Hamano]

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

> `.gitignore` file).
>  
> - - Otherwise, Git treats the pattern as a shell glob suitable
> -   for consumption by fnmatch(3) with the FNM_PATHNAME flag:
> -   wildcards in the pattern will not match a / in the pathname.
> -   For example, "Documentation/{asterisk}.html" matches
> -   "Documentation/git.html" but not "Documentation/ppc/ppc.html"
> -   or "tools/perf/Documentation/perf.html".
> + - Otherwise, Git treats the pattern as a shell glob: '{asterisk}'
> +   matches anything except '/', '?' matches any one character except
> +   '/' and '[]' matches one character in a selected range. See
> +   fnmatch(3) and the FNM_PATHNAME flag for a more accurate
> +   description.

Where the original did not quote single letters at all, this uses a
pair of single quotes.  Did you make sure it renders well in HTML
and manpage already or should I do that for you before applying?

I think what you wrote is accurate enough already, and those who
want to go to fnmatch(3) would do so not for accuracy but for
authority ;-) Perhaps s/accurate/detailed/?

Re: [PATCH v2 3/3] rebase: introduce and use pseudo-ref ORIG_COMMIT

[Junio C Hamano]

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

> The new command `git rebase --show-current-patch` is useful for seeing
> the commit related to the current rebase state. Some however may find
> the "git show" command behind it too limiting. You may want to
> increase context lines, do a diff that ignores whitespaces...
>
> For these advanced use cases, the user can execute any command they
> want with the new pseudo ref ORIG_COMMIT.
>
> This also helps show where the stopped commit is from, which is hard
> to see from the previous patch which implements --show-current-patch.
>
> Helped-by: Tim Landscheidt 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---

Hmph, how is this new file conceptually different from existing ones
like CHERRY_PICK_HEAD?  

>  Documentation/git-rebase.txt   | 3 ++-
>  builtin/am.c   | 4 
>  contrib/completion/git-completion.bash | 2 +-
>  git-rebase--interactive.sh | 5 -
>  git-rebase--merge.sh   | 4 +++-
>  git-rebase.sh  | 1 +
>  sequencer.c| 3 +++
>  t/t3400-rebase.sh  | 3 ++-
>  t/t3404-rebase-interactive.sh  | 5 -
>  9 files changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 7ef9577472..6da9296bf8 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -252,7 +252,8 @@ leave out at most one of A and B, in which case it 
> defaults to HEAD.
>  
>  --show-current-patch::
>   Show the current patch in an interactive rebase or when rebase
> - is stopped because of conflicts.
> + is stopped because of conflicts. This is the equivalent of
> + `git show ORIG_COMMIT`.
>  
>  -m::
>  --merge::
> diff --git a/builtin/am.c b/builtin/am.c
> index caec50cba9..bf9b356340 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -1011,6 +1011,7 @@ static void am_setup(struct am_state *state, enum 
> patch_format patch_format,
>  
>   if (mkdir(state->dir, 0777) < 0 && errno != EEXIST)
>   die_errno(_("failed to create directory '%s'"), state->dir);
> + delete_ref(NULL, "ORIG_COMMIT", NULL, REF_NO_DEREF);
>  
>   if (split_mail(state, patch_format, paths, keep_cr) < 0) {
>   am_destroy(state);
> @@ -1110,6 +,7 @@ static void am_next(struct am_state *state)
>  
>   oidclr(>orig_commit);
>   unlink(am_path(state, "original-commit"));
> + delete_ref(NULL, "ORIG_COMMIT", NULL, REF_NO_DEREF);
>  
>   if (!get_oid("HEAD", ))
>   write_state_text(state, "abort-safety", oid_to_hex());
> @@ -1441,6 +1443,8 @@ static int parse_mail_rebase(struct am_state *state, 
> const char *mail)
>  
>   oidcpy(>orig_commit, _oid);
>   write_state_text(state, "original-commit", oid_to_hex(_oid));
> + update_ref("am", "ORIG_COMMIT", _oid,
> +NULL, 0, UPDATE_REFS_DIE_ON_ERR);
>  
>   return 0;
>  }
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index 2bd30d68cf..deea688e0e 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -439,7 +439,7 @@ __git_refs ()
>   track=""
>   ;;
>   *)
> - for i in HEAD FETCH_HEAD ORIG_HEAD MERGE_HEAD; do
> + for i in HEAD FETCH_HEAD ORIG_HEAD MERGE_HEAD 
> ORIG_COMMIT; do
>   case "$i" in
>   $match*)
>   if [ -e "$dir/$i" ]; then
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 0c0f8abbf9..ef72bd5871 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -199,12 +199,14 @@ make_patch () {
>  
>  die_with_patch () {
>   echo "$1" > "$state_dir"/stopped-sha
> + git update-ref ORIG_COMMIT "$1"
>   make_patch "$1"
>   die "$2"
>  }
>  
>  exit_with_patch () {
>   echo "$1" > "$state_dir"/stopped-sha
> + git update-ref ORIG_COMMIT "$1"
>   make_patch $1
>   git rev-parse --verify HEAD > "$amend"
>   gpg_sign_opt_quoted=${gpg_sign_opt:+$(git rev-parse --sq-quote 
> "$gpg_sign_opt")}
> @@ -841,7 +843,7 @@ To continue rebase after editing, run:
>   exit
>   ;;
>  show-current-patch)
> - exec git show "$(cat "$state_dir/stopped-sha")" --
> + exec git show ORIG_COMMIT --
>   ;;
>  esac
>  
> @@ -858,6 +860,7 @@ fi
>  
>  orig_head=$(git rev-parse --verify HEAD) || die "$(gettext "No HEAD?")"
>  mkdir -p "$state_dir" || die "$(eval_gettext "Could not create temporary 
> \$state_dir")"
> +rm -f "$(git rev-parse --git-path ORIG_COMMIT)"
>  
>  : > "$state_dir"/interactive || die "$(gettext "Could not mark as 
> interactive")"
>  write_basic_state
> diff --git a/git-rebase--merge.sh 

Re: Git remote helper's fast export options

[Hareesh Rajan]

Hi,

I use git remote helper to export/import repository content. Git
internally uses fast-export command and it seems like the options to
detect move and copy (-M/-C) are not being used.
Logging this issue requesting a fix to the remote helper to generate
rename and copy commands in the output dump.

Git version: 2.14.1

Thanks,
Hareesh

Re: [PATCH v2 1/3] am: add --show-current-patch

[Junio C Hamano]

Eric Sunshine  writes:

> On Wed, Jan 31, 2018 at 4:30 AM, Nguyễn Thái Ngọc Duy  
> wrote:
>> Pointing the user to $GIT_DIR/rebase-apply may encourage them to mess
>> around in there, which is not a good thing. With this, the user does
>> not have to keep the path around somewhere (because after a couple of
>> commands, the path may be out of scrollback buffer) when they need to
>> look at the patch.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy 
>> ---
>> diff --git a/builtin/am.c b/builtin/am.c
>> @@ -2121,6 +2120,22 @@ static void am_abort(struct am_state *state)
>> +static int show_patch(struct am_state *state)
>> +{
>> +   struct strbuf sb = STRBUF_INIT;
>> +   int len;
>> +
>> +   len = strbuf_read_file(, am_path(state, msgnum(state)), 0);
>> +   if (len < 0)
>> +   die_errno(_("failed to read '%s'"),
>> + am_path(state, msgnum(state)));
>
> Isn't this am_path() invocation inside die_errno() likely to clobber
> the 'errno' from strbuf_read_file() which you want to be reporting?

True.  After coming up with the pathname to the current patch file,
we are going to exit without ever calling am_path(), or underlying
get_pathname() via mkpath(), again before exiting anyway, so perhaps
it is sufficient to just do an am_path() just once upfront, feed it
to strbuf_read_file() and also to die_errno().

Re: [PATCH v5 5/7] convert: add 'working-tree-encoding' attribute

[Junio C Hamano]

Lars Schneider  writes:

>> I am not sure why this is special cased and other codepaths have "if
>> WRITE_OBJECT then die, otherwise error" checks, so no, I do not
>> agree with your reasoning, at least not yet.
>
> The convert_to_git()/encode_to_git() machinery is used in two different
> kinds of code paths:
>
> Some code paths actually write to the Git database (indicated by the 
> CONV_WRITE_OBJECT flag). I consider these the "critical/important" code 
> paths and I don't want to tolerate any encoding errors in these cases as 
> the errors would be "forever" in the Git database. That's why I call 
> die() on errors for these cases to abort whatever we are doing. 
>
> Other code paths do not write to the Git database (e.g. during "git 
> checkout" we use the code to ensure that we are moving away from the 
> exact state that we think we are moving away). In these code paths I am 
> less concerned about encoding errors. I also don't want to abort the 
> operation (e.g. "git checkout") in these cases. That's why I only inform
> the user about the problem with an error message.

Warning the users early while they are doing non-writing
operation to give them chance to adjust the contents, before they
actually need to register the contents as objects by writing, at
which point we need to die.  That's a reasonable distinction and all
of that I already agree with.

What was questionable and left unexplained was why this roundtrip
thing needs to be different.

> The encoding round-trip check can be expensive. That's why I decided 
> initially to only execute the check in the "critical/important" 
> write-to-Git-database situations (CONV_WRITE_OBJECT flag!). I also 
> decided to run it only if the "SHIFT-JIS" encoding is used as this was
> the only encoding that I could find which reportedly does not round-trip
> with UTF-8 (although I was not able to replicate the round-trip 
> problems). 

I still do not see why you have problems with the approach of
maintaining a configurable set of "iffy" encodings (and throw SJIS
into the default list) to achieve all of the above and more.  For
SJIS users, instead of having to set environment variables to obtain
safe behaviour, they automatically get safe behaviour.  When using
encodings that are not problematic, they do not need to spend cycles
checking round-trip.  And when SJIS users know they do not care about
roundtrip checks, they can just configure SJIS away from the list.

Re: [PATCH v2 10/14] commit-graph: add core.commitgraph setting

[Igor Djordjevic]

Hi Derrick,

On 30/01/2018 22:39, Derrick Stolee wrote:
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 0e25b2c92b..5b63559a2b 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -898,6 +898,9 @@ 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::
 ^^^
A small style nitpick - you may want to use "core.commitGraph" 
throughout the series (note capital "G"), making it more readable and 
aligning with the rest of `git config` variable names (using "bumpyCaps" 
as per coding guidelines[1], and as seen a few lines below, at the 
end of this very patch, too, "core.sparseCheckout").

> + Enable git commit graph feature. Allows reading from .graph files.
> +
>  core.sparseCheckout::
>   Enable "sparse checkout" feature. See section "Sparse checkout" in
>   linkgit:git-read-tree[1] for more information.
> diff --git a/cache.h b/cache.h
> index d8b975a571..e50e447a4f 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -825,6 +825,7 @@ extern char *git_replace_ref_base;
>  extern int fsync_object_files;
>  extern int core_preload_index;
>  extern int core_apply_sparse_checkout;
> +extern int core_commitgraph;
^^^
Similar nit here, might be "core_commit_graph" (throughout the 
series) would align better with existing variable names around it 
(note additional underscore between "commit" and "graph"), but also 
with your own naming "scheme" used for cmd_commit_graph(), 
builtin_commit_graph_usage[], construct_commit_graph(), etc.

>  extern int precomposed_unicode;
>  extern int protect_hfs;
>  extern int protect_ntfs;
> diff --git a/config.c b/config.c
> index e617c2018d..99153fcfdb 100644
> --- a/config.c
> +++ b/config.c
> @@ -1223,6 +1223,11 @@ static int git_default_core_config(const char *var, 
> const char *value)
>   return 0;
>   }
>  
> + if (!strcmp(var, "core.commitgraph")) {
> + core_commitgraph = git_config_bool(var, value);
> + return 0;
> + }
> +
>   if (!strcmp(var, "core.sparsecheckout")) {
>   core_apply_sparse_checkout = git_config_bool(var, value);
>   return 0;
> diff --git a/environment.c b/environment.c
> index 63ac38a46f..faa4323cc5 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -61,6 +61,7 @@ enum object_creation_mode object_creation_mode = 
> OBJECT_CREATION_MODE;
>  char *notes_ref_name;
>  int grafts_replace_parents = 1;
>  int core_apply_sparse_checkout;
> +int core_commitgraph;
>  int merge_log_config = -1;
>  int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
>  unsigned long pack_size_limit_cfg;
> 

Thanks, Buga

[1] https://github.com/git/git/blob/master/Documentation/CodingGuidelines

  Externally Visible Names
  
  ...
  
  The section and variable names that consist of multiple words are
  formed by concatenating the words without punctuations (e.g. `-`),
  and are broken using bumpyCaps in documentation as a hint to the
  reader.

Re: [PATCH v2 14/27] connect: request remote refs using v2

[Derrick Stolee]

On 1/31/2018 3:10 PM, Eric Sunshine wrote:

On Wed, Jan 31, 2018 at 10:22 AM, Derrick Stolee  wrote:

On 1/25/2018 6:58 PM, Brandon Williams wrote:

  +static int process_ref_v2(const char *line, struct ref ***list)
+{
+   int ret = 1;
+   int i = 0;

nit: you set 'i' here, but first use it in a for loop with blank
initializer. Perhaps keep the first assignment closer to the first use?

Hmm, I see 'i' being incremented a couple times before the loop...


+   if (string_list_split(_sections, line, ' ', -1) < 2) {
+   ret = 0;
+   goto out;
+   }
+
+   if (get_oid_hex(line_sections.items[i++].string, _oid)) {

here...


+   ret = 0;
+   goto out;
+   }
+
+   ref = alloc_ref(line_sections.items[i++].string);

and here...


+
+   oidcpy(>old_oid, _oid);
+   **list = ref;
+   *list = >next;
+
+   for (; i < line_sections.nr; i++) {

then it is used in the loop.


+   const char *arg = line_sections.items[i].string;
+   if (skip_prefix(arg, "symref-target:", ))
+   ref->symref = xstrdup(arg);


Thanks! Sorry I missed this.

-Stolee

Re: Segmentation fault in git cherry-pick

[Elijah Newren]

On Wed, Jan 31, 2018 at 1:43 PM, Ayke van Laethem
 wrote:
> Hi all,
>
> I've found a segmentation fault in git.
>
> Here, fabsf is a branch that I'm trying to get the topmost commit from.
> After the failed cherry-pick, the change is added to the local working
> tree, but the commit isn't applied and .git/index.lock still exists.
>
> Version: 2.11.0 (Debian stretch)
>
> ~/src/micropython/ports/nrf$ valgrind git cherry-pick fabsf
> ==23286== Memcheck, a memory error detector
> ==23286== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
> ==23286== Using Valgrind-3.12.0.SVN and LibVEX; rerun with -h for copyright 
> info
> ==23286== Command: git cherry-pick fabsf
> ==23286==
> ==23286== Invalid read of size 4
> ==23286==at 0x21A348: add_index_entry_with_check (read-cache.c:1012)
> ==23286==by 0x21A348: add_index_entry (read-cache.c:1061)
> ==23286==by 0x1FAE85: merge_content (merge-recursive.c:1727)

This looks like the stack trace in
https://github.com/git-for-windows/git/issues/952, which was fixed by
Johannes Schindelin in commit
55e9f0e5c ("merge-recursive: handle NULL in add_cacheinfo()
correctly", 2016-11-26).  Could you retry with a newer version of git?

Elijah

Segmentation fault in git cherry-pick

[Ayke van Laethem]

Hi all,

I've found a segmentation fault in git.

Here, fabsf is a branch that I'm trying to get the topmost commit from.
After the failed cherry-pick, the change is added to the local working
tree, but the commit isn't applied and .git/index.lock still exists.

Version: 2.11.0 (Debian stretch)

~/src/micropython/ports/nrf$ valgrind git cherry-pick fabsf
==23286== Memcheck, a memory error detector
==23286== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==23286== Using Valgrind-3.12.0.SVN and LibVEX; rerun with -h for copyright info
==23286== Command: git cherry-pick fabsf
==23286==
==23286== Invalid read of size 4
==23286==at 0x21A348: add_index_entry_with_check (read-cache.c:1012)
==23286==by 0x21A348: add_index_entry (read-cache.c:1061)
==23286==by 0x1FAE85: merge_content (merge-recursive.c:1727)
==23286==by 0x1FBAAC: process_entry (merge-recursive.c:1885)
==23286==by 0x1FBAAC: merge_trees (merge-recursive.c:1948)
==23286==by 0x23E2F7: do_recursive_merge (sequencer.c:389)
==23286==by 0x23E2F7: do_pick_commit (sequencer.c:757)
==23286==by 0x23FB52: single_pick (sequencer.c:1329)
==23286==by 0x23FB52: sequencer_pick_revisions (sequencer.c:1378)
==23286==by 0x1874CE: run_sequencer (revert.c:178)
==23286==by 0x187927: cmd_cherry_pick (revert.c:203)
==23286==by 0x11A204: run_builtin (git.c:373)
==23286==by 0x11A204: handle_builtin (git.c:572)
==23286==by 0x11A5A1: run_argv (git.c:630)
==23286==by 0x11A5A1: cmd_main (git.c:707)
==23286==by 0x1195D1: main (common-main.c:40)
==23286==  Address 0x38 is not stack'd, malloc'd or (recently) free'd
==23286==
==23286==
==23286== Process terminating with default action of signal 11 (SIGSEGV)
==23286==  Access not within mapped region at address 0x38
==23286==at 0x21A348: add_index_entry_with_check (read-cache.c:1012)
==23286==by 0x21A348: add_index_entry (read-cache.c:1061)
==23286==by 0x1FAE85: merge_content (merge-recursive.c:1727)
==23286==by 0x1FBAAC: process_entry (merge-recursive.c:1885)
==23286==by 0x1FBAAC: merge_trees (merge-recursive.c:1948)
==23286==by 0x23E2F7: do_recursive_merge (sequencer.c:389)
==23286==by 0x23E2F7: do_pick_commit (sequencer.c:757)
==23286==by 0x23FB52: single_pick (sequencer.c:1329)
==23286==by 0x23FB52: sequencer_pick_revisions (sequencer.c:1378)
==23286==by 0x1874CE: run_sequencer (revert.c:178)
==23286==by 0x187927: cmd_cherry_pick (revert.c:203)
==23286==by 0x11A204: run_builtin (git.c:373)
==23286==by 0x11A204: handle_builtin (git.c:572)
==23286==by 0x11A5A1: run_argv (git.c:630)
==23286==by 0x11A5A1: cmd_main (git.c:707)
==23286==by 0x1195D1: main (common-main.c:40)

Note: I'm not subscribed to this mailing list.

--
Ayke

RE: Shawn Pearce has died

[Randall S. Becker]

On January 30, 2018 1:49 PM, Jeff King
@vger.kernel.org; sc...@gasch.org
> Subject: Re: Shawn Pearce has died
> On Mon, Jan 29, 2018 at 11:15:55PM -0800, Chris DiBona wrote:
> > That's a fantastic idea.  When is the contrib summit and is it open to
> > others? I could send someone ...
> 
> It's March 7th in Barcelona. Details are here:
> 
>   https://public-inbox.org/git/20180119001034.ga29...@sigill.intra.peff.net/
> 
> There will be GitHub video folks on premises the following day for the Git
> Merge main conference, but I'm looking into whether they'll be around to
> record a few interviews on the contributor summit day.

On behalf of git's NonStop platform team, ITUGLIB,  we want to express our 
sorrow for the loss of Shawn to the community, his friends, colleagues, and 
family. We take a small amount of solace in that his footprints will remain 
with us forever.

Sincerest Condolences,
Randall

--
Randall S. Becker
ITUGLIB Process Designer, Repository Manager, Git and Occasional Other Porting 
Dude
 NonStop developer since approximately 2112884442
 UNIX developer since approximately 421664400
-- In my real life, I talk too much.

Re: [PATCH v2 01/41] parse-options: support --git-completion-helper

[Eric Sunshine]

On Wed, Jan 31, 2018 at 6:05 AM, Nguyễn Thái Ngọc Duy  wrote:
> This option is designed to be used by git-completion.bash. For many
> simple cases, what we do in there is usually
>
> __gitcomp "lots of completion options"
>
> which has to be manually updated when a new user-visible option is
> added. With support from parse-options, we can write
>
> __gitcomp "$(git command --git-completion-helper)"
>
> and get that list directly from the parser for free. Dangerous/Unpopular
> options could be hidden with the new "NOCOMPLETE" flag.

I wonder if this option should be named DANGEROUS rather than
NOCOMPLETE to better reflect its intention. The reason I ask is that
it is easy to imagine git-completion.bash some day growing a new
configuration option to allow people to complete these "dangerous"
options, as well, rather than us imposing, with no escape hatch, our
idea of what should and should not complete.

It's not uncommon for "bug reports" to be sent to the list stating
that such-and-such option (say, --force) does not autocomplete. Our
stock answer is "oh, that's a dangerous option, so you'll have to type
it manually". If git-completion.bash gains new configuration to allow
dangerous options, then our answer can become "oh, that's a dangerous
option, if you really want it to complete, then enable
GIT_COMPLETION_DANGEROUS" (or whatever).

Re: [PATCH v2 13/41] completion: use __gitcomp_builtin in _git_commit

[Eric Sunshine]

On Wed, Jan 31, 2018 at 6:05 AM, Nguyễn Thái Ngọc Duy  wrote:
> The new comletable options are:

s/comletable/completable/

> --branch
> --gpg-sign
> --long
> --no-post-rewrite
> --null
> --porcelain
> --status
>
> --allow-empty is no longer completable because it's a hidden option
> since 4741edd549 (Remove deprecated OPTION_BOOLEAN for parsing arguments
> - 2013-08-03)
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 

Re: [PATCH v2 01/41] parse-options: support --git-completion-helper

[Eric Sunshine]

On Wed, Jan 31, 2018 at 6:05 AM, Nguyễn Thái Ngọc Duy  wrote:
> This option is designed to be used by git-completion.bash. For many
> simple cases, what we do in there is usually
>
> __gitcomp "lots of completion options"
>
> which has to be manually updated when a new user-visible option is
> added. With support from parse-options, we can write
>
> __gitcomp "$(git command --git-completion-helper)"
>
> and get that list directly from the parser for free. Dangerous/Unpopular
> options could be hidden with the new "NOCOMPLETE" flag.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/parse-options.h b/parse-options.h
> @@ -89,6 +90,8 @@ typedef int parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
> + *   PARSE_OPT_NOCOMPLETE: by default all visible options are completable
> + *git-completion.bash. This option suppresses that.

s/git-/by &/

Re: [PATCH v2 14/27] connect: request remote refs using v2

[Eric Sunshine]

On Wed, Jan 31, 2018 at 10:22 AM, Derrick Stolee  wrote:
> On 1/25/2018 6:58 PM, Brandon Williams wrote:
>>  +static int process_ref_v2(const char *line, struct ref ***list)
>> +{
>> +   int ret = 1;
>> +   int i = 0;
>
> nit: you set 'i' here, but first use it in a for loop with blank
> initializer. Perhaps keep the first assignment closer to the first use?

Hmm, I see 'i' being incremented a couple times before the loop...

>> +   if (string_list_split(_sections, line, ' ', -1) < 2) {
>> +   ret = 0;
>> +   goto out;
>> +   }
>> +
>> +   if (get_oid_hex(line_sections.items[i++].string, _oid)) {

here...

>> +   ret = 0;
>> +   goto out;
>> +   }
>> +
>> +   ref = alloc_ref(line_sections.items[i++].string);

and here...

>> +
>> +   oidcpy(>old_oid, _oid);
>> +   **list = ref;
>> +   *list = >next;
>> +
>> +   for (; i < line_sections.nr; i++) {

then it is used in the loop.

>> +   const char *arg = line_sections.items[i].string;
>> +   if (skip_prefix(arg, "symref-target:", ))
>> +   ref->symref = xstrdup(arg);

Re: [PATCH/RFC v5 7/7] Careful with CRLF when using e.g. UTF-16 for working-tree-encoding

[Lars Schneider]

> On 31 Jan 2018, at 18:28, Torsten Bögershausen  wrote:
> 
> []
>>> That is a good one.
>>> If you ever plan a re-roll (I don't at the moment) the *.proj extemsion
>>> make much more sense in Documentation/gitattributes that *.tx
>>> There no text files encoded in UTF-16 wich are called xxx.txt, but those
>>> are non-ideal examples. *.proj makes good sense as an example.
>> 
>> OK, I'll do that. Would that fix the problem which this patch tries to 
>> address for you?
>> (I would also explicitly add a paragraph to discuss line endings)
> 
> Please let me see the patch first, before I can have a comment.

Sure! I'll have it ready tomorrow.


> But back to the more general question:
> 
> How should Git handle the line endings of UTF-16 files in the woring-tree,
> which are UTF-8 in the index?
> 
> 
> There are 2 opposite opionions/user expectations here:
> 
> a) They are binary in the working tree, so git should leave the line endings
>   as is. (Unless specified otherwise in the .attributes file)

Well, if you consider your UTF-16 files binary then you would not change
*anything*. You would not enable the new "working-tree-encoding" attribute.
As a consequence, Git's behavior would not change. Git would leave all line 
endings as they are for the UTF-16 files.


> b) They are text files in the index. Git will convert line endings
>   if core.autocrlf is true (or the .gitattributes file specifies "-text")

This would *only* happen if you enable the new "working-tree-encoding"
attribute. In this case a user has already made the conscious decision to
treat these files as text files. Therefore, the user expects Git to handle
them in the same way other text files are handled.


> My feeling is that both arguments are valid, so let's ask for opinions
> and thoughts of others.
> Erik, Junio, Johannes, Johannes, Jeff, Ramsay, everybody:
> What do yo think ?

- Lars

Re: [PATCH v5 5/7] convert: add 'working-tree-encoding' attribute

[Lars Schneider]

> On 30 Jan 2018, at 22:56, Junio C Hamano  wrote:
> 
> Lars Schneider  writes:
> 
>>> On 30 Jan 2018, at 21:05, Junio C Hamano  wrote:
>>> 
>>> tbo...@web.de writes:
>>> 
 +  if ((conv_flags & CONV_WRITE_OBJECT) && !strcmp(enc->name, 
 "SHIFT-JIS")) {
 +  char *re_src;
 +  int re_src_len;
>>> 
>>> I think it is a bad idea to 
>>> 
>>> (1) not check without CONV_WRITE_OBJECT here.
>> 
>> The idea is to perform the roundtrip check *only* if we 
>> actually write to Git. In all other cases we don't care
>> if the encoding roundtrips.
>> 
>> "git checkout" is such a case where we don't care as 
>> noted by Peff here:
>> https://public-inbox.org/git/20171215095838.ga3...@sigill.intra.peff.net/
>> 
>> Do you agree?
> 
> I am not sure why this is special cased and other codepaths have "if
> WRITE_OBJECT then die, otherwise error" checks, so no, I do not
> agree with your reasoning, at least not yet.

The convert_to_git()/encode_to_git() machinery is used in two different
kinds of code paths:

Some code paths actually write to the Git database (indicated by the 
CONV_WRITE_OBJECT flag). I consider these the "critical/important" code 
paths and I don't want to tolerate any encoding errors in these cases as 
the errors would be "forever" in the Git database. That's why I call 
die() on errors for these cases to abort whatever we are doing. 

Other code paths do not write to the Git database (e.g. during "git 
checkout" we use the code to ensure that we are moving away from the 
exact state that we think we are moving away). In these code paths I am 
less concerned about encoding errors. I also don't want to abort the 
operation (e.g. "git checkout") in these cases. That's why I only inform
the user about the problem with an error message.

The encoding round-trip check can be expensive. That's why I decided 
initially to only execute the check in the "critical/important" 
write-to-Git-database situations (CONV_WRITE_OBJECT flag!). I also 
decided to run it only if the "SHIFT-JIS" encoding is used as this was
the only encoding that I could find which reportedly does not round-trip
with UTF-8 (although I was not able to replicate the round-trip 
problems). 

I want to change the current implementation as follows:

I want to check the round-trip encoding only if the the environment 
variable "GIT_WORKING_TREE_ENCODING_ROUNDTRIP_CHECK" is set. This way
a user can check the round-trip if necessary for *any* encoding. I
don't want to make it a git config because that setting should only 
rarely be used for debugging purposes.

Performing the round-trip check every time is not necessary from my 
point of view because it can be expensive and I was not able to generate
a test case which *does not* round-trip without triggering any other
iconv error.

- Lars

Re: [PATCH v2 02/10] sequencer: introduce new commands to reset therevision

[Phillip Wood]

On 31/01/18 13:21, Johannes Schindelin wrote:
> 
> Hi Stefan,
> 
> On Tue, 30 Jan 2018, Stefan Beller wrote:
> 
>> On Mon, Jan 29, 2018 at 2:54 PM, Johannes Schindelin
>>  wrote:
>>> @@ -116,6 +118,13 @@ static GIT_PATH_FUNC(rebase_path_stopped_sha, 
>>> "rebase-merge/stopped-sha")
>>>  static GIT_PATH_FUNC(rebase_path_rewritten_list, 
>>> "rebase-merge/rewritten-list")
>>>  static GIT_PATH_FUNC(rebase_path_rewritten_pending,
>>> "rebase-merge/rewritten-pending")
>>> +
>>> +/*
>>> + * The path of the file listing refs that need to be deleted after the 
>>> rebase
>>> + * finishes. This is used by the `merge` command.
>>> + */
> 
> Whoops. The comment "This is used by the `merge` command`" is completely
> wrong. Will fix.
> 
>> So this file contains (label -> commit),
> 
> Only `label`. No `commit`.
> 
>> which is appended in do_label, it uses refs to store the commits in
>> refs/rewritten.  We do not have to worry about the contents of that file
>> getting too long, or label re-use, because the directory containing all
>> these helper files will be deleted upon successful rebase in
>> `sequencer_remove_state()`.
> 
> Yes.
>
It might be a good idea to have 'git rebase --abort' delete the refs as
well as the file though

Best Wishes

Phillip

Re: [PATCH 2/8] sequencer: introduce the `merge` command

[Phillip Wood]

On 31/01/18 13:48, Johannes Schindelin wrote:
> Hi Jake & Phillip,
> 
> On Mon, 29 Jan 2018, Johannes Schindelin wrote:
> 
>> On Sat, 20 Jan 2018, Jacob Keller wrote:
>>
>>> On Fri, Jan 19, 2018 at 6:45 AM, Phillip Wood  
>>> wrote:
 On 18/01/18 15:35, Johannes Schindelin wrote:
>
> This patch adds the `merge` command, with the following syntax:
>
>   merge   

 I'm concerned that this will be confusing for users. All of the other
 rebase commands replay the changes in the commit hash immediately
 following the command name. This command instead uses the first
 commit to specify the message which is different to both 'git merge'
 and the existing rebase commands. I wonder if it would be clearer to
 have 'merge -C   ...' instead so it's clear which
 argument specifies the message and which the remote head to merge.
 It would also allow for 'merge -c   ...' in the future
 for rewording an existing merge message and also avoid the slightly
 odd 'merge -  ...'. Where it's creating new merges I'm not sure
 it's a good idea to encourage people to only have oneline commit
 messages by making it harder to edit them, perhaps it could take
 another argument to mean open the editor or not, though as Jake said
 I guess it's not that common.
>>>
>>> I actually like the idea of re-using commit message options like -C,
>>> -c,  and -m, so we could do:
>>>
>>> merge -C  ... to take message from commit
>>
>> That is exactly how the Git garden shears do it.
>>
>> I found it not very readable. That is why I wanted to get away from it in
>> --recreate-merges.
> 
> I made up my mind. Even if it is not very readable, it is still better
> than the `merge A B` where the order of A and B magically determines their
> respective roles.
> 
>>> merge -c  ...  to take the message from commit and open editor to 
>>> edit
>>> merge -m "" ... to take the message from the quoted test
>>> merge ... to merge and open commit editor with default message
> 
> I will probably implement -c, but not -m, and will handle the absence of
> the -C and -c options to construct a default merge message which can then
> be edited.

That sounds like a good plan (-c can always be added later), I'm really
pleased you changed your mind on this, having the -C may be a bit ugly
but I think it is valuable to have some way of distinguishing the
message commit from the merge heads.

> The -m option just opens such a can of worms with dequoting, that's why I
> do not want to do that.
> 
> BTW I am still trying to figure out how to present the oneline of the
> commit to merge (which is sometimes really helpful because the label might
> be less than meaningful) while *still* allowing for octopus merges.
> 
> So far, what I have is this:
> 
>   merge   
> 
> and for octopus:
> 
>   merge  " ..." ...
> 
> I think with the -C syntax, it would become something like
> 
>   merge -C   # 
> 
> and
> 
>   merge -C   ...
>   # Merging: 
>   # Merging: 
>   # ...
> 
> The only qualm I have about this is that `#` really *is* a valid ref name.
> (Seriously, it is...). So that would mean that I'd have to disallow `#`
> as a label specificially.
> 
> Thoughts?

As ':' is not a valid ref if you want a separator you could have

merge -C   : 

personally I'm not sure what value having a separator adds in this case.
I think in the octopus case have a separate comment line for the subject
of each merge head is a good idea - maybe the two head merge could just
have the subject of the remote head in a comment below. I wonder if
having the subject of the commit that is going to be used for the
message may be a useful prompt in some cases but that's just making
things more complicated.

Best Wishes

Phillip

> Ciao,
> Dscho
> 

Re: Bug/comment

[Kaartic Sivaraam]

On Tuesday 30 January 2018 05:32 AM, Andrew Ardill wrote:
> Hi Ilija,
> 
> On 30 January 2018 at 10:21, Ilija Pecelj  wrote:
>> Though it might not be considered a bug 'per se' it is definitely wired.
>> Namely, when you type 'yes' word and hit enter in git bash for widnows, the
>> process enters infinite loop and just prints 'y' letter in new line.
>
> ...
>
> I agree it's a little weird if you have no idea what it's doing, but
> it is very useful and very old, used by many many different scripts
> etc, and so unlikely to change.
>

Just to make things more clear, 'yes' is an UNIX utility (as hinted in
the Wikipedia article link) that might come as part of Cygwin and is not
a part of Git itself.


-- 
Kaartic

QUOTE

“It is impossible to live without failing at something, unless you live
so cautiously that you might as well not have lived at all – in which
case, you fail by default.”

  - J. K. Rowling


WIKIPEDIA: DID YOU KNOW?

Only 33% of internet users in India have heard of Wikipedia !!

* What do you think could be the reason behind this?

* What are ways in which the awareness about Wikipedia in India and
other countries be increased ?

Reference:

* Give your ideas for increasing the awareness of Wikipedia in India and
in other countries and get a Grant from the Wikimedia Foundation to
bring your idea to life.

  https://meta.wikimedia.org/wiki/Grants:IdeaLab/Inspire

* Know more about the awareness problem of Wikipedia

  https://meta.wikimedia.org/wiki/New_Readers/Awareness

  https://meta.wikimedia.org/wiki/New_Readers/Next_steps/Raising_awareness



signature.asc
Description: OpenPGP digital signature

Re: [PATCH/RFC v5 7/7] Careful with CRLF when using e.g. UTF-16 for working-tree-encoding

[Torsten Bögershausen]

[]
> > That is a good one.
> > If you ever plan a re-roll (I don't at the moment) the *.proj extemsion
> > make much more sense in Documentation/gitattributes that *.tx
> > There no text files encoded in UTF-16 wich are called xxx.txt, but those
> > are non-ideal examples. *.proj makes good sense as an example.
> 
> OK, I'll do that. Would that fix the problem which this patch tries to 
> address for you?
> (I would also explicitly add a paragraph to discuss line endings)

Please let me see the patch first, before I can have a comment.

But back to the more general question:

How should Git handle the line endings of UTF-16 files in the woring-tree,
which are UTF-8 in the index?


There are 2 opposite opionions/user expectations here:

a) They are binary in the working tree, so git should leave the line endings
   as is. (Unless specified otherwise in the .attributes file)
b) They are text files in the index. Git will convert line endings
   if core.autocrlf is true (or the .gitattributes file specifies "-text")

My feeling is that both arguments are valid, so let's ask for opinions
and thoughts of others.
Erik, Junio, Johannes, Johannes, Jeff, Ramsay, everybody:
What do yo think ?

Re: Location limits on development, staging and production environments

[H]

On 01/30/2018 03:48 PM, H wrote:
> On 01/29/2018 10:02 PM, Bryan Turner wrote:
>> On Mon, Jan 29, 2018 at 11:08 AM, H  wrote:
>>> I am a newcomer to git looking to set up a web development environment 
>>> where individual computers are used for development, the development.git, 
>>> staging.git and production.git repositories are stored on an external 
>>> server reachable by password-less ssh and the staging and production 
>>> websites are on yet another server, also reachable by password-less ssh 
>>> from the git-server (and the development machines).
>>>
>>> Locating the three git repositories on an external server works fine but I 
>>> have not been able to have the staging and production deployment files on 
>>> another server. I believe this is what is referred by GIT_WORK_TREE and 
>>> based on what I found on the web I created a post-receive hook of 
>>> staging.git with the two lines:
>>>
>>> #!/bin/sh
>>> GIT_WORK_TREE=user@1.2.3.4:/var/www/html/dev.whatever git checkout -f master
>>>
>>> I believe this should deploy the files from the development work tree.
>>>
>>> The above, however, fails. Should it work? I am running git 1.7.1 on CentOS 
>>> 6.
>> No, I wouldn't expect that to work. GIT_WORK_TREE is not remote-aware
>> in that way. It's expected to be a normal-ish filesystem path.
>>
>> Based on your description, and the hook you've written, it seems like
>> your intention is for the source to automatically be fetched and
>> checked out on the staging environment after each push. (This is
>> dangerous, and likely _not_ what you actually want, but I'll get to
>> that in a moment.)
>>
>> One option would be to setup something like NFS, so the git-server can
>> mount the filesystems from the staging and production nodes.
>>
>> A different, likely better, option would be to have the post-receive
>> script on the git-server use straight ssh to trigger a checkout script
>> on the staging server, e.g.:
>> #!/bin/sh
>> ssh example@staging-server -C /opt/deploy-staging.sh
>>
>> Your deploy-staging script would then do something like:
>> #!/bin/sh
>> GIT_WORK_TREE=/var/www/html/dev.whatever git pull origin
>>
>> That said, though, having such a simple script is dangerous because
>> Git is fully capable of having receiving multiple pushes concurrently,
>> and they can all succeed as long as they're updating different
>> branches. Since your script isn't considering what branches were
>> changed by the push, it could end up triggering simultaneous git
>> processes on the staging server all attempting to deploy concurrently.
>>
>> The stdin for the post-receive hook receives details about which refs
>> were changed, and you'll likely want to update your script to parse
>> stdin and only try to deploy staging if a specific, relevant branch
>> (master in your example) has changed.
>>
>> Lastly, I'll note that using post-receive will make the pushing
>> (remote) user wait while the staging server is deployed. If that
>> process is likely to take very long, you might want to decouple the
>> two somehow.
>>
>> Hope this helps!
> I should perhaps also have mentioned that although I am the only developer, I 
> may use different computers to develop on. IOW, there should not be any 
> conflict due to code being pushed by multiple developers.
>
> Let's see if I understand this correctly:
>
> - Unless NFS is used, the git archive and the deployment of the website code 
> in this case should reside on the same computer.
>
> - The combination of the checkout script and the deploy-staging script should 
> work provided not multiple updates to the same branch are pushed at the same 
> time.
>
> I will try this later today but any other hints or suggestions you may have 
> would be greatly appreciated!
>
I modified post-receive in the hook directory of the staging git archive on 
server 1 as per your first example and created the deploy-staging.sh script on 
server 2 where the staging website resides in /var/www/html/dev.whatever (as 
well as the live website in /var/www/html/whatever. However, when trying out 
the deploy-staging.sh script, it fails with the error message "...not a git 
repository...". I do have git on server 2 but what else would I need to set up 
to run this successfully? Note that the git repository is on server 1 and I am 
deploying to server 2.

Thanks!

Re: Some rough edges of core.fsmonitor

[Ben Peart]

On 1/30/2018 6:16 PM, Ævar Arnfjörð Bjarmason wrote:


On Tue, Jan 30 2018, Ben Peart jotted:


While some of these issues have been discussed in other threads, I
thought I'd summarize my thoughts here.


Thanks for this & your other reply. I'm going to get to testing some of
Duy's patches soon, and if you have some other relevant WIP I'd be happy
to try them, but meanwhile replying to a few of these:


On 1/26/2018 7:28 PM, Ævar Arnfjörð Bjarmason wrote:

I just got around to testing this since it landed, for context some
previous poking of mine in [1].

Issues / stuff I've noticed:

1) We end up invalidating the untracked cache because stuff in .git/
changed. For example:

  01:09:24.975524 fsmonitor.c:173 fsmonitor process 
'.git/hooks/fsmonitor-watchman' returned success
  01:09:24.975548 fsmonitor.c:138 fsmonitor_refresh_callback '.git'
  01:09:24.975556 fsmonitor.c:138 fsmonitor_refresh_callback 
'.git/config'
  01:09:24.975568 fsmonitor.c:138 fsmonitor_refresh_callback 
'.git/index'
  01:09:25.122726 fsmonitor.c:91  write fsmonitor extension 
successful

Am I missing something or should we do something like:

  diff --git a/fsmonitor.c b/fsmonitor.c
  index 0af7c4edba..5067b89bda 100644
  --- a/fsmonitor.c
  +++ b/fsmonitor.c
  @@ -118,7 +118,12 @@ static int query_fsmonitor(int version, uint64_t 
last_update, struct strbuf *que

   static void fsmonitor_refresh_callback(struct index_state *istate, const 
char *name)
   {
  -   int pos = index_name_pos(istate, name, strlen(name));
  +   int pos;
  +
  +   if (!strcmp(name, ".git") || starts_with(name, ".git/"))
  +   return;
  +
  +   pos = index_name_pos(istate, name, strlen(name));

  if (pos >= 0) {
  struct cache_entry *ce = istate->cache[pos];

With that patch applied status on a large repo[2] goes from a consistent
~180-200ms to ~140-150ms, since we're not invalidating some of the UC
structure



I favor making this optimization by updating
untracked_cache_invalidate_path() so that it ignores paths under
get_git_dir() and doesn't invalidate the untracked cache or flag the
index as dirty.


*nod*


2) We re-write out the index even though we know nothing changed

When you first run with core.fsmonitor it needs to
mark_fsmonitor_clean() for every path, but is there a reason for why we
wouldn't supply the equivalent of GIT_OPTIONAL_LOCKS=0 if all paths are
marked and we know from the hook that nothing changed? Why write out the
index again?



Writing out the index when core.fsmonitor is first turned on is
necessary to get the index extension added with the current state of
the dirty flags.  Given it is a one time cost, I don't think we have
anything worth trying to optimize here.


Indeed, that makes sense. What I was showing here is even after the
initial setup we continue to write it out when we know nothing changed.

We do that anyway without fsmonitor, but this seemed like a worthwhile
thing to optimize.



There is already logic to avoid writing out the index unless there is 
something that requires it.  In my testing, it was often the untracked 
cache marking the index as dirty that was causing the unexpected writes.


The patch to make the untracked cache only flag the index as dirty when 
the feature is being turned on or off is pretty simple (see below).  The 
challenge was that many of the untracked cache tests assume all changes 
are saved to disk after every command so they fail after making this 
change.  The patch below does a simple hack of checking for a test 
environment variable and only forcing the index writes if it is set.


Internally, we simply turned off the untracked cache as it's usefulness 
in combination with GVFS is limited and without the patch, it actually 
slowed down common operations more than it sped them up.


Typically, changes to the untracked cache don't accumulate long before 
the user does some operation that requires the index to be written out 
at which point the untracked cache is updated as well.



diff --git a/dir.c b/dir.c
index 5e93a1350b..af1d33aae1 100644
--- a/dir.c
+++ b/dir.c
@@ -2256,7 +2256,8 @@ int read_directory(struct dir_struct *dir, struct 
index_state *istate,

 dir->untracked->gitignore_invalidated,
 dir->untracked->dir_invalidated,
 dir->untracked->dir_opened);
-   if (dir->untracked == istate->untracked &&
+   if (getenv("GIT_TEST_UNTRACKED_CACHE") &&
+   dir->untracked == istate->untracked &&
(dir->untracked->dir_opened ||
 dir->untracked->gitignore_invalidated ||
 dir->untracked->dir_invalidated))
diff --git a/t/t7063-status-untracked-cache.sh 
b/t/t7063-status-untracked-cache.sh

index 

Re: [PATCH v2 00/27] protocol version 2

[Derrick Stolee]

Sorry for chiming in with mostly nitpicks so late since sending this 
version. Mostly, I tried to read it to see if I could understand the 
scope of the patch and how this code worked before. It looks very 
polished, so I the nits were the best I could do.


On 1/25/2018 6:58 PM, Brandon Williams wrote:

Changes in v2:
  * Added documentation for fetch
  * changes #defines for state variables to be enums
  * couple code changes to pkt-line functions and documentation
  * Added unit tests for the git-serve binary as well as for ls-refs


I'm a fan of more unit-level testing, and I think that will be more 
important as we go on with these multiple configuration options.



Areas for improvement
  * Push isn't implemented, right now this is ok because if v2 is requested the
server can just default to v0.  Before this can be merged we may want to
change how the client request a new protocol, and not allow for sending
"version=2" when pushing even though the user has it configured.  Or maybe
its fine to just have an older client who doesn't understand how to push
(and request v2) to die if the server tries to speak v2 at it.

Fixing this essentially would just require piping through a bit more
information to the function which ultimately runs connect (for both builtins
and remote-curl)


Definitely save push for a later patch. Getting 'fetch' online did 
require 'ls-refs' at the same time. Future reviews will be easier when 
adding one command at a time.




  * I want to make sure that the docs are well written before this gets merged
so I'm hoping that someone can do a through review on the docs themselves to
make sure they are clear.


I made a comment in the docs about the architectural changes. While I 
think a discussion on that topic would be valuable, I'm not sure that's 
the point of the document (i.e. documenting what v2 does versus selling 
the value of the patch). I thought the docs were clear for how the 
commands work.



  * Right now there is a capability 'stateless-rpc' which essentially makes sure
that a server command completes after a single round (this is to make sure
http works cleanly).  After talking with some folks it may make more sense
to just have v2 be stateless in nature so that all commands terminate after
a single round trip.  This makes things a bit easier if a server wants to
have ssh just be a proxy for http.

One potential thing would be to flip this so that by default the protocol is
stateless and if a server/command has a state-full mode that can be
implemented as a capability at a later point.  Thoughts?


At minimum, all commands should be designed with a "stateless first" 
philosophy since a large number of users communicate via HTTP[S] and any 
decisions that make stateless communication painful should be rejected.



  * Shallow repositories and shallow clones aren't supported yet.  I'm working
on it and it can be either added to v2 by default if people think it needs
to be in there from the start, or we can add it as a capability at a later
point.


I'm happy to say the following:

1. Shallow repositories should not be used for servers, since they 
cannot service all requests.


2. Since v2 has easy capability features, I'm happy to leave shallow for 
later. We will want to verify that a shallow clone command reverts to v1.



I fetched bw/protocol-v2 with tip 13c70148, built, set 
'protocol.version=2' in the config, and tested fetches against GitHub 
and VSTS just as a compatibility test. Everything worked just fine.


Is there an easy way to test the existing test suite for clone and fetch 
using protocol v2 to make sure there are no regressions with 
protocol.version=2 in the config?


Thanks,
-Stolee

Re: [PATCH v2 12/27] serve: introduce git-serve

[Derrick Stolee]

On 1/25/2018 6:58 PM, Brandon Williams wrote:

Introduce git-serve, the base server for protocol version 2.

Protocol version 2 is intended to be a replacement for Git's current
wire protocol.  The intention is that it will be a simpler, less
wasteful protocol which can evolve over time.

Protocol version 2 improves upon version 1 by eliminating the initial
ref advertisement.  In its place a server will export a list of
capabilities and commands which it supports in a capability
advertisement.  A client can then request that a particular command be
executed by providing a number of capabilities and command specific
parameters.  At the completion of a command, a client can request that
another command be executed or can terminate the connection by sending a
flush packet.

Signed-off-by: Brandon Williams 
---
  .gitignore  |   1 +
  Documentation/technical/protocol-v2.txt | 117 +++
  Makefile|   2 +
  builtin.h   |   1 +
  builtin/serve.c |  30 
  git.c   |   1 +
  serve.c | 249 
  serve.h |  15 ++
  t/t5701-git-serve.sh|  56 +++
  9 files changed, 472 insertions(+)
  create mode 100644 Documentation/technical/protocol-v2.txt
  create mode 100644 builtin/serve.c
  create mode 100644 serve.c
  create mode 100644 serve.h
  create mode 100755 t/t5701-git-serve.sh

diff --git a/.gitignore b/.gitignore
index 833ef3b0b..2d0450c26 100644
--- a/.gitignore
+++ b/.gitignore
@@ -140,6 +140,7 @@
  /git-rm
  /git-send-email
  /git-send-pack
+/git-serve
  /git-sh-i18n
  /git-sh-i18n--envsubst
  /git-sh-setup
diff --git a/Documentation/technical/protocol-v2.txt 
b/Documentation/technical/protocol-v2.txt
new file mode 100644
index 0..7f619a76c
--- /dev/null
+++ b/Documentation/technical/protocol-v2.txt
@@ -0,0 +1,117 @@
+ Git Wire Protocol, Version 2
+==
+
+This document presents a specification for a version 2 of Git's wire
+protocol.  Protocol v2 will improve upon v1 in the following ways:
+
+  * Instead of multiple service names, multiple commands will be
+supported by a single service.


As someone unfamiliar with the old protocol code, this statement is 
underselling the architectural significance of your change. The new 
model allows a single service to handle all different wire protocols 
(git://, ssh://, https://) while being agnostic to the command-specific 
logic. It also hides the protocol negotiation away from these consumers.


The ease with which you are adding new commands in later commits really 
demonstrates the value of this patch. To make that point here, you would 
almost need to document the old model to show how it was difficult to 
use and extend. Perhaps this document will not need expanding since the 
code speaks for itself.


I just wanted to state for the record that the new architecture is a big 
improvement and will make more commands much easier to implement.



+  * Easily extendable as capabilities are moved into their own section
+of the protocol, no longer being hidden behind a NUL byte and
+limited by the size of a pkt-line (as there will be a single
+capability per pkt-line).
+  * Separate out other information hidden behind NUL bytes (e.g. agent
+string as a capability and symrefs can be requested using 'ls-refs')
+  * Reference advertisement will be omitted unless explicitly requested
+  * ls-refs command to explicitly request some refs
+


nit: some bullets have full stops (.) and others do not.


+ Detailed Design
+=
+
+A client can request to speak protocol v2 by sending `version=2` in the
+side-channel `GIT_PROTOCOL` in the initial request to the server.
+
+In protocol v2 communication is command oriented.  When first contacting a
+server a list of capabilities will advertised.  Some of these capabilities
+will be commands which a client can request be executed.  Once a command
+has completed, a client can reuse the connection and request that other
+commands be executed.
+
+ Special Packets
+-
+
+In protocol v2 these special packets will have the following semantics:
+
+  * '' Flush Packet (flush-pkt) - indicates the end of a message
+  * '0001' Delimiter Packet (delim-pkt) - separates sections of a message
+
+ Capability Advertisement
+--
+
+A server which decides to communicate (based on a request from a client)
+using protocol version 2, notifies the client by sending a version string
+in its initial response followed by an advertisement of its capabilities.
+Each capability is a key with an optional value.  Clients must ignore all
+unknown keys.  Semantics of unknown values are left to the definition of
+each key.  Some capabilities will describe commands which can be 

Re: [PATCH v2 14/27] connect: request remote refs using v2

[Derrick Stolee]

On 1/25/2018 6:58 PM, Brandon Williams wrote:

Teach the client to be able to request a remote's refs using protocol
v2.  This is done by having a client issue a 'ls-refs' request to a v2
server.

Signed-off-by: Brandon Williams 
---
  builtin/upload-pack.c  |  10 ++--
  connect.c  | 123 -
  remote.h   |   4 ++
  t/t5702-protocol-v2.sh |  28 +++
  transport.c|   2 +-
  5 files changed, 160 insertions(+), 7 deletions(-)
  create mode 100755 t/t5702-protocol-v2.sh

diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c
index 8d53e9794..a757df8da 100644
--- a/builtin/upload-pack.c
+++ b/builtin/upload-pack.c
@@ -5,6 +5,7 @@
  #include "parse-options.h"
  #include "protocol.h"
  #include "upload-pack.h"
+#include "serve.h"
  
  static const char * const upload_pack_usage[] = {

N_("git upload-pack [] "),
@@ -16,6 +17,7 @@ int cmd_upload_pack(int argc, const char **argv, const char 
*prefix)
const char *dir;
int strict = 0;
struct upload_pack_options opts = { 0 };
+   struct serve_options serve_opts = SERVE_OPTIONS_INIT;
struct option options[] = {
OPT_BOOL(0, "stateless-rpc", _rpc,
 N_("quit after a single request/response exchange")),
@@ -48,11 +50,9 @@ int cmd_upload_pack(int argc, const char **argv, const char 
*prefix)
  
  	switch (determine_protocol_version_server()) {

case protocol_v2:
-   /*
-* fetch support for protocol v2 has not been implemented yet,
-* so ignore the request to use v2 and fallback to using v0.
-*/
-   upload_pack();
+   serve_opts.advertise_capabilities = opts.advertise_refs;
+   serve_opts.stateless_rpc = opts.stateless_rpc;
+   serve(_opts);
break;
case protocol_v1:
/*
diff --git a/connect.c b/connect.c
index f2157a821..3c653b65b 100644
--- a/connect.c
+++ b/connect.c
@@ -12,9 +12,11 @@
  #include "sha1-array.h"
  #include "transport.h"
  #include "strbuf.h"
+#include "version.h"
  #include "protocol.h"
  
  static char *server_capabilities;

+static struct argv_array server_capabilities_v2 = ARGV_ARRAY_INIT;
  static const char *parse_feature_value(const char *, const char *, int *);
  
  static int check_ref(const char *name, unsigned int flags)

@@ -62,6 +64,33 @@ static void die_initial_contact(int unexpected)
  "and the repository exists."));
  }
  
+/* Checks if the server supports the capability 'c' */

+static int server_supports_v2(const char *c, int die_on_error)
+{
+   int i;
+
+   for (i = 0; i < server_capabilities_v2.argc; i++) {
+   const char *out;
+   if (skip_prefix(server_capabilities_v2.argv[i], c, ) &&
+   (!*out || *out == '='))
+   return 1;
+   }
+
+   if (die_on_error)
+   die("server doesn't support '%s'", c);
+
+   return 0;
+}
+
+static void process_capabilities_v2(struct packet_reader *reader)
+{
+   while (packet_reader_read(reader) == PACKET_READ_NORMAL)
+   argv_array_push(_capabilities_v2, reader->line);
+
+   if (reader->status != PACKET_READ_FLUSH)
+   die("protocol error");
+}
+
  enum protocol_version discover_version(struct packet_reader *reader)
  {
enum protocol_version version = protocol_unknown_version;
@@ -85,7 +114,7 @@ enum protocol_version discover_version(struct packet_reader 
*reader)
/* Maybe process capabilities here, at least for v2 */
switch (version) {
case protocol_v2:
-   die("support for protocol v2 not implemented yet");
+   process_capabilities_v2(reader);
break;
case protocol_v1:
/* Read the peeked version line */
@@ -293,6 +322,98 @@ struct ref **get_remote_heads(struct packet_reader *reader,
return list;
  }
  
+static int process_ref_v2(const char *line, struct ref ***list)

+{
+   int ret = 1;
+   int i = 0;


nit: you set 'i' here, but first use it in a for loop with blank 
initializer. Perhaps keep the first assignment closer to the first use?



+   struct object_id old_oid;
+   struct ref *ref;
+   struct string_list line_sections = STRING_LIST_INIT_DUP;
+
+   if (string_list_split(_sections, line, ' ', -1) < 2) {
+   ret = 0;
+   goto out;
+   }
+
+   if (get_oid_hex(line_sections.items[i++].string, _oid)) {
+   ret = 0;
+   goto out;
+   }
+
+   ref = alloc_ref(line_sections.items[i++].string);
+
+   oidcpy(>old_oid, _oid);
+   **list = ref;
+   *list = >next;
+
+   for (; i < line_sections.nr; i++) {
+   const char *arg = line_sections.items[i].string;
+   if (skip_prefix(arg, 

Re: [PATCH v2 00/41] Automate updating git-completion.bash a bit

[Ævar Arnfjörð Bjarmason]

On Wed, Jan 31 2018, Nguyễn Thái Ngọc Duy jotted:

> I posted a proof of concept a while back [1]. This is the full version.
>
> This series lets "git" binary help git-completion.bash to complete
> -- so that when a new option is added, we don't have to update
> git-completion.bash manually too (people often forget it). As a side
> effect, about 180 more options are now completable.
>
> parse-options is updated to allow developers to flag certain options
> not to be completable if they want finer control over it.  But by
> default, new non-hidden options are completable. Negative forms must
> be handled manually. That's for the next step.
>
> The number of patches is high, but changes after the first four
> patches and 33/41 are quite simple. I still need some eyeballs though
> to make sure I have not accidentally allowed completion of dangerous
> options. Details are broken down per command in each commit message.
>
> If people want to play with this, I have a script [2] that shows all
> completable options for most commands (I ignore some that are
> shell-based because I don't touch them in this series). You can then
> do a "diff" to see new/old options.

Thanks, looks great to me, especially caching the result of the
completion.

> There's a small conflict with 'pu' because --prune-tags is added in
> git-completion.bash. The solution is simple and beautiful: ignore
> those changes, --prune-tags will be completable anyway :)

Yay! Also another good argument for this series, it took me until v3
until I noticed I'd forgotten the bash completion:
https://public-inbox.org/git/20180123221326.28495-1-ava...@gmail.com/

> parse-options: add OPT_xxx_F() variants

Not directly related to this series, but my own
https://public-inbox.org/git/20170324231013.23346-1-ava...@gmail.com/
which I've been meaning to clean up and re-submit also added some new
macros to this file.

I've been wondering what a good solution is to avoid a combinatorial
explosion explosion of these macros in the longer term, but haven't come
up with anthing.

Re: [PATCH v2 10/27] protocol: introduce enum protocol_version value protocol_v2

[Derrick Stolee]

On 1/25/2018 6:58 PM, Brandon Williams wrote:

Introduce protocol_v2, a new value for 'enum protocol_version'.
Subsequent patches will fill in the implementation of protocol_v2.

Signed-off-by: Brandon Williams 
---
  builtin/fetch-pack.c   | 3 +++
  builtin/receive-pack.c | 6 ++
  builtin/send-pack.c| 3 +++
  builtin/upload-pack.c  | 7 +++
  connect.c  | 3 +++
  protocol.c | 2 ++
  protocol.h | 1 +
  remote-curl.c  | 3 +++
  transport.c| 9 +
  9 files changed, 37 insertions(+)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 85d4faf76..f492e8abd 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -201,6 +201,9 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
   PACKET_READ_GENTLE_ON_EOF);
  
  	switch (discover_version()) {

+   case protocol_v2:
+   die("support for protocol v2 not implemented yet");
+   break;
case protocol_v1:
case protocol_v0:
get_remote_heads(, , 0, NULL, );
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index b7ce7c7f5..3656e94fd 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1963,6 +1963,12 @@ int cmd_receive_pack(int argc, const char **argv, const 
char *prefix)
unpack_limit = receive_unpack_limit;
  
  	switch (determine_protocol_version_server()) {

+   case protocol_v2:
+   /*
+* push support for protocol v2 has not been implemented yet,
+* so ignore the request to use v2 and fallback to using v0.
+*/
+   break;
case protocol_v1:
/*
 * v1 is just the original protocol with a version string,
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 83cb125a6..b5427f75e 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -263,6 +263,9 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
   PACKET_READ_GENTLE_ON_EOF);
  
  	switch (discover_version()) {

+   case protocol_v2:
+   die("support for protocol v2 not implemented yet");
+   break;
case protocol_v1:
case protocol_v0:
get_remote_heads(, _refs, REF_NORMAL,
diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c
index 2cb5cb35b..8d53e9794 100644
--- a/builtin/upload-pack.c
+++ b/builtin/upload-pack.c
@@ -47,6 +47,13 @@ int cmd_upload_pack(int argc, const char **argv, const char 
*prefix)
die("'%s' does not appear to be a git repository", dir);
  
  	switch (determine_protocol_version_server()) {

+   case protocol_v2:
+   /*
+* fetch support for protocol v2 has not been implemented yet,
+* so ignore the request to use v2 and fallback to using v0.
+*/
+   upload_pack();
+   break;
case protocol_v1:
/*
 * v1 is just the original protocol with a version string,
diff --git a/connect.c b/connect.c
index db3c9d24c..f2157a821 100644
--- a/connect.c
+++ b/connect.c
@@ -84,6 +84,9 @@ enum protocol_version discover_version(struct packet_reader 
*reader)
  
  	/* Maybe process capabilities here, at least for v2 */

switch (version) {
+   case protocol_v2:
+   die("support for protocol v2 not implemented yet");
+   break;
case protocol_v1:
/* Read the peeked version line */
packet_reader_read(reader);
diff --git a/protocol.c b/protocol.c
index 43012b7eb..5e636785d 100644
--- a/protocol.c
+++ b/protocol.c
@@ -8,6 +8,8 @@ static enum protocol_version parse_protocol_version(const char 
*value)
return protocol_v0;
else if (!strcmp(value, "1"))
return protocol_v1;
+   else if (!strcmp(value, "2"))
+   return protocol_v2;
else
return protocol_unknown_version;
  }
diff --git a/protocol.h b/protocol.h
index 1b2bc94a8..2ad35e433 100644
--- a/protocol.h
+++ b/protocol.h
@@ -5,6 +5,7 @@ enum protocol_version {
protocol_unknown_version = -1,
protocol_v0 = 0,
protocol_v1 = 1,
+   protocol_v2 = 2,
  };
  
  /*

diff --git a/remote-curl.c b/remote-curl.c
index 9f6d07683..dae8a4a48 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -185,6 +185,9 @@ static struct ref *parse_git_refs(struct discovery *heads, 
int for_push)
   PACKET_READ_GENTLE_ON_EOF);
  
  	switch (discover_version()) {

+   case protocol_v2:
+   die("support for protocol v2 not implemented yet");
+   break;
case protocol_v1:
case protocol_v0:
get_remote_heads(, , for_push ? REF_NORMAL : 0,
diff --git a/transport.c b/transport.c
index 2378dcb38..83d9dd1df 100644
--- 

Re: [PATCH v2 09/27] transport: store protocol version

[Derrick Stolee]

On 1/25/2018 6:58 PM, Brandon Williams wrote:

+   switch (data->version) {
+   case protocol_v1:
+   case protocol_v0:
+   refs = fetch_pack(, data->fd, data->conn,
+ refs_tmp ? refs_tmp : transport->remote_refs,
+ dest, to_fetch, nr_heads, >shallow,
+ >pack_lockfile);
+   break;
+   case protocol_unknown_version:
+   BUG("unknown protocol version");
+   }


After seeing this pattern a few times, I think it would be good to 
convert it to a macro that calls a statement for protocol_v1/v0 (and 
later calls a different one for protocol_v2). It would at minimum reduce 
the code clones surrounding this handling of unknown_version, and we 
could have one place that is clear this BUG() is due to an unexpected 
response from discover_version().

Re: [PATCH v2 08/27] connect: discover protocol version outside of get_remote_heads

[Derrick Stolee]

On 1/25/2018 6:58 PM, Brandon Williams wrote:

In order to prepare for the addition of protocol_v2 push the protocol
version discovery outside of 'get_remote_heads()'.  This will allow for
keeping the logic for processing the reference advertisement for
protocol_v1 and protocol_v0 separate from the logic for protocol_v2.

Signed-off-by: Brandon Williams 
---
  builtin/fetch-pack.c | 16 +++-
  builtin/send-pack.c  | 17 +++--
  connect.c| 27 ++-
  connect.h|  3 +++
  remote-curl.c| 20 ++--
  remote.h |  5 +++--
  transport.c  | 24 +++-
  7 files changed, 83 insertions(+), 29 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 366b9d13f..85d4faf76 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -4,6 +4,7 @@
  #include "remote.h"
  #include "connect.h"
  #include "sha1-array.h"
+#include "protocol.h"
  
  static const char fetch_pack_usage[] =

  "git fetch-pack [--all] [--stdin] [--quiet | -q] [--keep | -k] [--thin] "
@@ -52,6 +53,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
struct fetch_pack_args args;
struct oid_array shallow = OID_ARRAY_INIT;
struct string_list deepen_not = STRING_LIST_INIT_DUP;
+   struct packet_reader reader;
  
  	packet_trace_identity("fetch-pack");
  
@@ -193,7 +195,19 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)

if (!conn)
return args.diag_url ? 0 : 1;
}
-   get_remote_heads(fd[0], NULL, 0, , 0, NULL, );
+
+   packet_reader_init(, fd[0], NULL, 0,
+  PACKET_READ_CHOMP_NEWLINE |
+  PACKET_READ_GENTLE_ON_EOF);
+
+   switch (discover_version()) {
+   case protocol_v1:
+   case protocol_v0:
+   get_remote_heads(, , 0, NULL, );
+   break;
+   case protocol_unknown_version:
+   BUG("unknown protocol version");


Is this really a BUG in the client, or a bug/incompatibility in the server?

Perhaps I'm misunderstanding, but it looks like discover_version() will 
die() on an unknown version (the die() is in 
protocol.c:determine_protocol_version_client()). So maybe that's why 
this is a BUG()?


If there is something to change here, this BUG() appears three more times.


+   }
  
  	ref = fetch_pack(, fd, conn, ref, dest, sought, nr_sought,

 , pack_lockfile_ptr);
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index fc4f0bb5f..83cb125a6 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -14,6 +14,7 @@
  #include "sha1-array.h"
  #include "gpg-interface.h"
  #include "gettext.h"
+#include "protocol.h"
  
  static const char * const send_pack_usage[] = {

N_("git send-pack [--all | --mirror] [--dry-run] [--force] "
@@ -154,6 +155,7 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
int progress = -1;
int from_stdin = 0;
struct push_cas_option cas = {0};
+   struct packet_reader reader;
  
  	struct option options[] = {

OPT__VERBOSITY(),
@@ -256,8 +258,19 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
args.verbose ? CONNECT_VERBOSE : 0);
}
  
-	get_remote_heads(fd[0], NULL, 0, _refs, REF_NORMAL,

-_have, );
+   packet_reader_init(, fd[0], NULL, 0,
+  PACKET_READ_CHOMP_NEWLINE |
+  PACKET_READ_GENTLE_ON_EOF);
+
+   switch (discover_version()) {
+   case protocol_v1:
+   case protocol_v0:
+   get_remote_heads(, _refs, REF_NORMAL,
+_have, );
+   break;
+   case protocol_unknown_version:
+   BUG("unknown protocol version");
+   }
  
  	transport_verify_remote_names(nr_refspecs, refspecs);
  
diff --git a/connect.c b/connect.c

index 00e90075c..db3c9d24c 100644
--- a/connect.c
+++ b/connect.c
@@ -62,7 +62,7 @@ static void die_initial_contact(int unexpected)
  "and the repository exists."));
  }
  
-static enum protocol_version discover_version(struct packet_reader *reader)

+enum protocol_version discover_version(struct packet_reader *reader)
  {
enum protocol_version version = protocol_unknown_version;
  
@@ -234,7 +234,7 @@ enum get_remote_heads_state {

  /*
   * Read all the refs from the other end
   */
-struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
+struct ref **get_remote_heads(struct packet_reader *reader,
  struct ref **list, unsigned int flags,
  struct oid_array *extra_have,
  struct oid_array *shallow_points)
@@ -242,24 +242,17 @@ struct ref **get_remote_heads(int in, char *src_buf, 
size_t 

[no subject]

[Mario Ernesto Gerala]

You have been awarded a donation of $350,000 USD please reply this email for 
more info : sungla...@gmail.com

Re: [PATCH v2 05/27] upload-pack: factor out processing lines

[Derrick Stolee]

On 1/26/2018 4:33 PM, Brandon Williams wrote:

On 01/26, Stefan Beller wrote:

On Thu, Jan 25, 2018 at 3:58 PM, Brandon Williams  wrote:

Factor out the logic for processing shallow, deepen, deepen_since, and
deepen_not lines into their own functions to simplify the
'receive_needs()' function in addition to making it easier to reuse some
of this logic when implementing protocol_v2.

Signed-off-by: Brandon Williams 
---
  upload-pack.c | 113 ++
  1 file changed, 74 insertions(+), 39 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index 2ad73a98b..42d83d5b1 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -724,6 +724,75 @@ static void deepen_by_rev_list(int ac, const char **av,
 packet_flush(1);
  }

+static int process_shallow(const char *line, struct object_array *shallows)
+{
+   const char *arg;
+   if (skip_prefix(line, "shallow ", )) {

stylistic nit:

 You could invert the condition in each of the process_* functions
 to just have

 if (!skip_prefix...))
 return 0

 /* less indented code goes here */

 return 1;

 That way we have less indentation as well as easier code.
 (The reader doesn't need to keep in mind what the else
 part is about; it is a rather local decision to bail out instead
 of having the return at the end of the function.)

I was trying to move the existing code into helper functions so
rewriting them in transit may make it less reviewable?


I think the way you kept to the existing code as much as possible is 
good and easier to review. Perhaps a style pass after the patch lands is 
good for #leftoverbits.





+   struct object_id oid;
+   struct object *object;
+   if (get_oid_hex(arg, ))
+   die("invalid shallow line: %s", line);
+   object = parse_object();
+   if (!object)
+   return 1;
+   if (object->type != OBJ_COMMIT)
+   die("invalid shallow object %s", oid_to_hex());
+   if (!(object->flags & CLIENT_SHALLOW)) {
+   object->flags |= CLIENT_SHALLOW;
+   add_object_array(object, NULL, shallows);
+   }
+   return 1;
+   }
+
+   return 0;
+}
+
+static int process_deepen(const char *line, int *depth)
+{
+   const char *arg;
+   if (skip_prefix(line, "deepen ", )) {
+   char *end = NULL;
+   *depth = (int) strtol(arg, , 0);


nit: space between (int) and strtol?


+   if (!end || *end || *depth <= 0)
+   die("Invalid deepen: %s", line);
+   return 1;
+   }
+
+   return 0;
+}
+
+static int process_deepen_since(const char *line, timestamp_t *deepen_since, 
int *deepen_rev_list)
+{
+   const char *arg;
+   if (skip_prefix(line, "deepen-since ", )) {
+   char *end = NULL;
+   *deepen_since = parse_timestamp(arg, , 0);
+   if (!end || *end || !deepen_since ||
+   /* revisions.c's max_age -1 is special */
+   *deepen_since == -1)
+   die("Invalid deepen-since: %s", line);
+   *deepen_rev_list = 1;
+   return 1;
+   }
+   return 0;
+}
+
+static int process_deepen_not(const char *line, struct string_list 
*deepen_not, int *deepen_rev_list)
+{
+   const char *arg;
+   if (skip_prefix(line, "deepen-not ", )) {
+   char *ref = NULL;
+   struct object_id oid;
+   if (expand_ref(arg, strlen(arg), , ) != 1)
+   die("git upload-pack: ambiguous deepen-not: %s", line);
+   string_list_append(deepen_not, ref);
+   free(ref);
+   *deepen_rev_list = 1;
+   return 1;
+   }
+   return 0;
+}
+
  static void receive_needs(void)
  {
 struct object_array shallows = OBJECT_ARRAY_INIT;
@@ -745,49 +814,15 @@ static void receive_needs(void)
 if (!line)
 break;

-   if (skip_prefix(line, "shallow ", )) {
-   struct object_id oid;
-   struct object *object;
-   if (get_oid_hex(arg, ))
-   die("invalid shallow line: %s", line);
-   object = parse_object();
-   if (!object)
-   continue;
-   if (object->type != OBJ_COMMIT)
-   die("invalid shallow object %s", 
oid_to_hex());
-   if (!(object->flags & CLIENT_SHALLOW)) {
-   object->flags |= CLIENT_SHALLOW;
-   add_object_array(object, NULL, );
-   }
+   if (process_shallow(line, ))
 

Re: [PATCH 2/8] sequencer: introduce the `merge` command

[Johannes Schindelin]

Hi Jake & Phillip,

On Mon, 29 Jan 2018, Johannes Schindelin wrote:

> On Sat, 20 Jan 2018, Jacob Keller wrote:
> 
> > On Fri, Jan 19, 2018 at 6:45 AM, Phillip Wood  
> > wrote:
> > > On 18/01/18 15:35, Johannes Schindelin wrote:
> > >>
> > >> This patch adds the `merge` command, with the following syntax:
> > >>
> > >>   merge   
> > >
> > > I'm concerned that this will be confusing for users. All of the other
> > > rebase commands replay the changes in the commit hash immediately
> > > following the command name. This command instead uses the first
> > > commit to specify the message which is different to both 'git merge'
> > > and the existing rebase commands. I wonder if it would be clearer to
> > > have 'merge -C   ...' instead so it's clear which
> > > argument specifies the message and which the remote head to merge.
> > > It would also allow for 'merge -c   ...' in the future
> > > for rewording an existing merge message and also avoid the slightly
> > > odd 'merge -  ...'. Where it's creating new merges I'm not sure
> > > it's a good idea to encourage people to only have oneline commit
> > > messages by making it harder to edit them, perhaps it could take
> > > another argument to mean open the editor or not, though as Jake said
> > > I guess it's not that common.
> > 
> > I actually like the idea of re-using commit message options like -C,
> > -c,  and -m, so we could do:
> > 
> > merge -C  ... to take message from commit
> 
> That is exactly how the Git garden shears do it.
> 
> I found it not very readable. That is why I wanted to get away from it in
> --recreate-merges.

I made up my mind. Even if it is not very readable, it is still better
than the `merge A B` where the order of A and B magically determines their
respective roles.

> > merge -c  ...  to take the message from commit and open editor to 
> > edit
> > merge -m "" ... to take the message from the quoted test
> > merge ... to merge and open commit editor with default message

I will probably implement -c, but not -m, and will handle the absence of
the -C and -c options to construct a default merge message which can then
be edited.

The -m option just opens such a can of worms with dequoting, that's why I
do not want to do that.

BTW I am still trying to figure out how to present the oneline of the
commit to merge (which is sometimes really helpful because the label might
be less than meaningful) while *still* allowing for octopus merges.

So far, what I have is this:

merge   

and for octopus:

merge  " ..." ...

I think with the -C syntax, it would become something like

merge -C   # 

and

merge -C   ...
# Merging: 
# Merging: 
# ...

The only qualm I have about this is that `#` really *is* a valid ref name.
(Seriously, it is...). So that would mean that I'd have to disallow `#`
as a label specificially.

Thoughts?

Ciao,
Dscho

Re: [PATCH v2 00/10] rebase -i: offer to recreate merge commits

[Johannes Schindelin]

Hi Junio,

On Tue, 30 Jan 2018, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > Changes since v1:
> >
> > - reintroduced "sequencer: make refs generated by the `label` command
> >   worktree-local" (which was squashed into "sequencer: handle autosquash
> >   and post-rewrite for merge commands" by accident)
> 
> Good.
> 
> > - got rid of the universally-hated `bud` command
> 
> Universally is a bit too strong a word, unless you want to hint that
> you are specifically ignoring my input ;-).

In the interest of comic effect, I exaggerated a little.

> > - the no-rebase-cousins mode was made the default
> 
> Although I lack first-hand experience with this implementation, this
> design decision matches my instinct.

Excellent.

> May comment on individual patches separately, later.

I think I may want to introduce a bigger change, still. I forgot who
exactly came up with the suggestion to use `merge -C 
` (I think it was Jake), and I reacted too forcefully in
rejecting it.

This design had been my original design in the Git garden shears, and I
did not like it because it felt clunky and it also broke the style of
 .

But the longer I think about this, the more I come to the conclusion that
I was wrong, and that the -C way is the way that leaves the door open to
the pretty elegant `-c ` (imitating `git commit`'s option to
borrow the commit message from elsewhere but still allowing to edit it).

And it also leaves open the door to just write `merge ` and have
the sequencer come up with a default merge message that the user can then
edit.

I'll probably refrain from implementing support for -m because I do not
want to implement the dequoting.

Ciao,
Dscho

Re: [PATCH v2 02/10] sequencer: introduce new commands to reset the revision

[Johannes Schindelin]

Hi Stefan,

On Tue, 30 Jan 2018, Stefan Beller wrote:

> On Mon, Jan 29, 2018 at 2:54 PM, Johannes Schindelin
>  wrote:
> > @@ -116,6 +118,13 @@ static GIT_PATH_FUNC(rebase_path_stopped_sha, 
> > "rebase-merge/stopped-sha")
> >  static GIT_PATH_FUNC(rebase_path_rewritten_list, 
> > "rebase-merge/rewritten-list")
> >  static GIT_PATH_FUNC(rebase_path_rewritten_pending,
> > "rebase-merge/rewritten-pending")
> > +
> > +/*
> > + * The path of the file listing refs that need to be deleted after the 
> > rebase
> > + * finishes. This is used by the `merge` command.
> > + */

Whoops. The comment "This is used by the `merge` command`" is completely
wrong. Will fix.

> So this file contains (label -> commit),

Only `label`. No `commit`.

> which is appended in do_label, it uses refs to store the commits in
> refs/rewritten.  We do not have to worry about the contents of that file
> getting too long, or label re-use, because the directory containing all
> these helper files will be deleted upon successful rebase in
> `sequencer_remove_state()`.

Yes.

> > +static int do_reset(const char *name, int len)
> > +{
> > +   struct strbuf ref_name = STRBUF_INIT;
> > +   struct object_id oid;
> > +   struct lock_file lock = LOCK_INIT;
> > +   struct tree_desc desc;
> > +   struct tree *tree;
> > +   struct unpack_trees_options opts;
> > +   int ret = 0, i;
> > +
> > +   if (hold_locked_index(, LOCK_REPORT_ON_ERROR) < 0)
> > +   return -1;
> > +
> > +   /* Determine the length of the label */
> > +   for (i = 0; i < len; i++)
> > +   if (isspace(name[i]))
> > +   len = i;
> > +
> > +   strbuf_addf(_name, "refs/rewritten/%.*s", len, name);
> > +   if (get_oid(ref_name.buf, ) &&
> > +   get_oid(ref_name.buf + strlen("refs/rewritten/"), )) {
> > +   error(_("could not read '%s'"), ref_name.buf);
> > +   rollback_lock_file();
> > +   strbuf_release(_name);
> > +   return -1;
> > +   }
> > +
> > +   memset(, 0, sizeof(opts));
> > +   opts.head_idx = 1;
> > +   opts.src_index = _index;
> > +   opts.dst_index = _index;
> > +   opts.fn = oneway_merge;
> > +   opts.merge = 1;
> > +   opts.update = 1;
> > +   opts.reset = 1;
> > +
> > +   read_cache_unmerged();
> 
> In read-tree.c merge.c and pull.c we guard this conditionally
> and use die_resolve_conflict to bail out. In am.c we do not.
> 
> I think we'd want to guard it here, too?

Yes.

> Constructing an instruction sheet that produces a merge
> conflict just before the reset is a bit artificial, but still:
> 
> label onto
> pick abc
> exec false # run "git merge out-of-rebase-merge"
> # manually to produce a conflict

This would fail already, as `exec` tests for a clean index after the
operation ran.

> reset onto # we want to stop here telling the user to fix it.

But you are absolutely right that we still need to fix it.

> > +   if (!fill_tree_descriptor(, )) {
> > +   error(_("failed to find tree of %s"), oid_to_hex());
> > +   rollback_lock_file();
> > +   free((void *)desc.buffer);
> > +   strbuf_release(_name);
> > +   return -1;
> > +   }
> > +
> > +   if (unpack_trees(1, , )) {
> > +   rollback_lock_file();
> > +   free((void *)desc.buffer);
> > +   strbuf_release(_name);
> > +   return -1;
> > +   }
> > +
> > +   tree = parse_tree_indirect();
> > +   prime_cache_tree(_index, tree);
> > +
> > +   if (write_locked_index(_index, , COMMIT_LOCK) < 0)
> > +   ret = error(_("could not write index"));
> > +   free((void *)desc.buffer);
> 
> For most newer structs we have a {release, clear, free}_X,
> but for tree_desc's this seems to be the convention to avoid memleaks.

Yep, this code is just copy-edited from elsewhere. It seemed to be
different enough from the (very generic) use case in builtin/reset.c that
I did not think refactoring this into a convenience function in
unpack-trees.[ch] would make sense.

Ciao,
Dscho

Re: [PATCH v2 00/10] rebase -i: offer to recreate merge commits

[Johannes Schindelin]

Hi Stefan,

On Tue, 30 Jan 2018, Stefan Beller wrote:

> > - got rid of the universally-hated `bud` command
> 
> Sorry if you got the impression for that. Maybe I was imprecise.

You were not the most vocal voice. Anyway, `bud` is gone now.

> > Stefan Beller (1):
> >   git-rebase--interactive: clarify arguments
> 
> No need to honor me with authorship, as I just wrote
> that patch in a quick hurry to express the idea.

You wrote it.

> The interdiff looks good to me, I'll review the patches now.

Thanks,
Dscho

Re: Bug Report: Subtrees and GPG Signed Commits

[Stephen R Guglielmo]

On Tue, Jan 30, 2018 at 6:37 PM, Avery Pennarun  wrote:
> On Tue, Jan 30, 2018 at 6:24 PM, Junio C Hamano  wrote:
>> Stefan Beller  writes:
>>> There has not been feedback for a while on this thread.
>>> I think that is because subtrees are not in anyone's hot
>>> interest area currently.
>>>
>>> This is definitely the right place to submit bugs.
>>> Looking through "git log --format="%ae %s" -S subtree",
>>> it seems as if Avery (apenw...@gmail.com) was mostly
>>> interested in developing subtrees, though I think he has
>>> moved on. Originally it was invented by Junio, who is
>>> the active maintainer of the project in 68faf68938
>>> (A new merge stragety 'subtree'., 2007-02-15)
>>
>> Thanks for trying to help, but I have *NOTHING* to do with the "git
>> subtree" subcommand (and I personally have no interest in it).  What
>> I did was a subtree merge strategy (i.e. "git merge -s subtree"),
>> which is totally a different thing.
>>
>> David Greene offered to take it over in 2015, and then we saw some
>> activity by David Aguilar in 2016, but otherwise the subcommand from
>> contrib/ has pretty much been dormant these days.
>
> Strictly speaking, the 'git subtree' command does in fact use 'git
> merge -s subtree' under the covers, so Junio is at least partly
> responsible for giving me the idea :)
>
> I actually have never looked into how signed commits work and although
> I still use git-subtree occasionally (it hasn't needed any
> maintenance, for my simple use cases), I have never used it with
> signed commits.
>
> git-subtree maintains a cache that maps commit ids in the "original
> project" with their equivalents in the "merged project."  If there's
> something magic about how commit ids work with signed commits, I could
> imagine that causing the "no a valid object name" problems.  Or,
> git-subtree in --squash mode actually generates new commit objects
> using some magic of its own.  If it were to accidentally copy a
> signature into a commit that no longer matches the original, I imagine
> that new object might get rejected.
>
> Unfortunately I don't have time to look into it.  The git-subtree code
> is pretty straightforward, though, so if Stephen has an hour or two to
> look deeper it's probably possible to fix it up.  The tool is not
> actually as magical and difficult as it might seem at first glance :)
>
> Sorry I can't help more.
>
> Good luck,
>
> Avery

Thanks all for the discussion/replies.

We use subtrees extensively in our environment right now. The "sub"
repos (90+) are located on GitHub, while the "main/parent" repo is
provided by a vendor on website hosting infrastructure.

I will take a look at:
git/Documentation/CodingGuidelines
git/Documentation/SubmittingPatches
git/contrib/subtree/

Should I follow up in this thread with a patch (it might be a while)?

Thanks!
Steve

[PATCH v2 20/41] completion: use __gitcomp_builtin in _git_grep

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

The new completable options are:

--after-context=
--before-context=
--color
--context
--exclude-standard
--quiet
--recurse-submodules
--textconv

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/grep.c | 13 -
 contrib/completion/git-completion.bash | 16 +---
 2 files changed, 9 insertions(+), 20 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 3ca4ac80d8..496f6e 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -832,8 +832,9 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
OPT_BOOL('L', "files-without-match",
_name_only,
N_("show only the names of files without match")),
-   OPT_BOOL('z', "null", _following_name,
-   N_("print NUL after filenames")),
+   OPT_BOOL_F('z', "null", _following_name,
+  N_("print NUL after filenames"),
+  PARSE_OPT_NOCOMPLETE),
OPT_BOOL('c', "count", ,
N_("show the number of matches instead of matching 
lines")),
OPT__COLOR(, N_("highlight matches")),
@@ -884,9 +885,11 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
OPT_GROUP(""),
{ OPTION_STRING, 'O', "open-files-in-pager", _in_pager,
N_("pager"), N_("show matching files in the pager"),
-   PARSE_OPT_OPTARG, NULL, (intptr_t)default_pager },
-   OPT_BOOL(0, "ext-grep", _grep_allowed__ignored,
-N_("allow calling of grep(1) (ignored by this 
build)")),
+   PARSE_OPT_OPTARG | PARSE_OPT_NOCOMPLETE,
+   NULL, (intptr_t)default_pager },
+   OPT_BOOL_F(0, "ext-grep", _grep_allowed__ignored,
+  N_("allow calling of grep(1) (ignored by this 
build)"),
+  PARSE_OPT_NOCOMPLETE),
OPT_END()
};
 
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index f694331cfc..6e78737a13 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1551,21 +1551,7 @@ _git_grep ()
 
case "$cur" in
--*)
-   __gitcomp "
-   --cached
-   --text --ignore-case --word-regexp --invert-match
-   --full-name --line-number
-   --extended-regexp --basic-regexp --fixed-strings
-   --perl-regexp
-   --threads
-   --files-with-matches --name-only
-   --files-without-match
-   --max-depth
-   --count
-   --and --or --not --all-match
-   --break --heading --show-function --function-context
-   --untracked --no-index
-   "
+   __gitcomp_builtin grep
return
;;
esac
-- 
2.16.1.205.g271f633410

[PATCH v2 41/41] completion: use __gitcomp_builtin in _git_worktree

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

The new completable options for "worktree add" are:

--checkout
--guess-remote
--lock
--track

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/worktree.c | 2 +-
 contrib/completion/git-completion.bash | 8 
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 4d3422f62e..76dc6b8cb5 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -367,7 +367,7 @@ static int add(int ac, const char **av, const char *prefix)
struct option options[] = {
OPT__FORCE(,
   N_("checkout  even if already checked out in 
other worktree"),
-  0),
+  PARSE_OPT_NOCOMPLETE),
OPT_STRING('b', NULL, _branch, N_("branch"),
   N_("create a new branch")),
OPT_STRING('B', NULL, _branch_force, N_("branch"),
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 179ddd8c44..cfd24c5764 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3004,16 +3004,16 @@ _git_worktree ()
else
case "$subcommand,$cur" in
add,--*)
-   __gitcomp "--detach"
+   __gitcomp_builtin worktree_add
;;
list,--*)
-   __gitcomp "--porcelain"
+   __gitcomp_builtin worktree_list
;;
lock,--*)
-   __gitcomp "--reason"
+   __gitcomp_builtin worktree_lock
;;
prune,--*)
-   __gitcomp "--dry-run --expire --verbose"
+   __gitcomp_builtin worktree_prune
;;
*)
;;
-- 
2.16.1.205.g271f633410

[PATCH v2 40/41] completion: use __gitcomp_builtin in _git_tag

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

The new completable options are:

--color
--format=
--ignore-case

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 contrib/completion/git-completion.bash | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index ba9438e437..179ddd8c44 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2985,11 +2985,7 @@ _git_tag ()
 
case "$cur" in
--*)
-   __gitcomp "
-   --list --delete --verify --annotate --message --file
-   --sign --cleanup --local-user --force --column --sort=
-   --contains --no-contains --points-at --merged 
--no-merged --create-reflog
-   "
+   __gitcomp_builtin tag
;;
esac
 }
-- 
2.16.1.205.g271f633410

[PATCH v2 38/41] completion: use __gitcomp_builtin in _git_show_branch

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

No new completable options!

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 contrib/completion/git-completion.bash | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index e5a9f7f9b4..3722c6444a 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2756,12 +2756,7 @@ _git_show_branch ()
 {
case "$cur" in
--*)
-   __gitcomp "
-   --all --remotes --topo-order --date-order --current 
--more=
-   --list --independent --merge-base --no-name
-   --color --no-color
-   --sha1-name --sparse --topics --reflog
-   "
+   __gitcomp_builtin show-branch "--no-color"
return
;;
esac
-- 
2.16.1.205.g271f633410

[PATCH v2 39/41] completion: use __gitcomp_builtin in _git_status

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

The new completable options are --null and --show-stash.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 contrib/completion/git-completion.bash | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 3722c6444a..ba9438e437 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2052,11 +2052,7 @@ _git_status ()
return
;;
--*)
-   __gitcomp "
-   --short --branch --porcelain --long --verbose
-   --untracked-files= --ignore-submodules= --ignored
-   --column= --no-column
-   "
+   __gitcomp_builtin status "--no-column"
return
;;
esac
-- 
2.16.1.205.g271f633410

[PATCH v2 37/41] completion: use __gitcomp_builtin in _git_rm

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

No new completable options!

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/rm.c   | 2 +-
 contrib/completion/git-completion.bash | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/rm.c b/builtin/rm.c
index 6e0c7f5ac6..a818efe230 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -242,7 +242,7 @@ static struct option builtin_rm_options[] = {
OPT__DRY_RUN(_only, N_("dry run")),
OPT__QUIET(, N_("do not list removed files")),
OPT_BOOL( 0 , "cached", _only, N_("only remove from the 
index")),
-   OPT__FORCE(, N_("override the up-to-date check"), 0),
+   OPT__FORCE(, N_("override the up-to-date check"), 
PARSE_OPT_NOCOMPLETE),
OPT_BOOL('r', NULL, ,  N_("allow recursive 
removal")),
OPT_BOOL( 0 , "ignore-unmatch", _unmatch,
N_("exit with a zero status even if nothing 
matched")),
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index d6215c494e..e5a9f7f9b4 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2698,7 +2698,7 @@ _git_rm ()
 {
case "$cur" in
--*)
-   __gitcomp "--cached --dry-run --ignore-unmatch --quiet"
+   __gitcomp_builtin rm
return
;;
esac
-- 
2.16.1.205.g271f633410

[PATCH v2 33/41] remote: force completing --mirror= instead of --mirror

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

"git remote --mirror" is a special case. Technically it is possible to
specify --mirror without any argument. But we will get a "dangerous,
deprecated!" warning in that case.

This new parse-opt flag allows --git-completion-helper to always
complete --mirror=, ignoring the dangerous use case.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/remote.c | 2 +-
 parse-options.c  | 2 ++
 parse-options.h  | 6 +-
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index d95bf904c3..fce9e5c0f6 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -168,7 +168,7 @@ static int add(int argc, const char **argv)
OPT_STRING('m', "master", , N_("branch"), N_("master 
branch")),
{ OPTION_CALLBACK, 0, "mirror", , N_("push|fetch"),
N_("set up remote as a mirror to push to or fetch 
from"),
-   PARSE_OPT_OPTARG, parse_mirror_opt },
+   PARSE_OPT_OPTARG | PARSE_OPT_COMP_ARG, parse_mirror_opt 
},
OPT_END()
};
 
diff --git a/parse-options.c b/parse-options.c
index 29f4defdd6..979577ba2c 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -459,6 +459,8 @@ static int show_gitcomp(struct parse_opt_ctx_t *ctx,
default:
break;
}
+   if (opts->flags & PARSE_OPT_COMP_ARG)
+   suffix = "=";
printf(" --%s%s", opts->long_name, suffix);
}
fputc('\n', stdout);
diff --git a/parse-options.h b/parse-options.h
index fa75df17b4..f63151fbda 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -39,7 +39,8 @@ enum parse_opt_option_flags {
PARSE_OPT_NODASH = 32,
PARSE_OPT_LITERAL_ARGHELP = 64,
PARSE_OPT_SHELL_EVAL = 256,
-   PARSE_OPT_NOCOMPLETE = 512
+   PARSE_OPT_NOCOMPLETE = 512,
+   PARSE_OPT_COMP_ARG = 1024
 };
 
 struct option;
@@ -92,6 +93,9 @@ typedef int parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
  * Useful for options with multiple parameters.
  *   PARSE_OPT_NOCOMPLETE: by default all visible options are completable
  *git-completion.bash. This option suppresses that.
+ *   PARSE_OPT_COMP_ARG: this option forces to git-completion.bash to
+ *  complete an option as --name= not --name even if
+ *  the option takes optional argument.
  *
  * `callback`::
  *   pointer to the callback to use for OPTION_CALLBACK or
-- 
2.16.1.205.g271f633410

[PATCH v2 36/41] completion: use __gitcomp_builtin in _git_revert

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

The new completable option is --gpg-sign

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/revert.c   | 12 +---
 contrib/completion/git-completion.bash |  5 +
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index b9d927eb09..c8e045911b 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -96,9 +96,15 @@ static int run_sequencer(int argc, const char **argv, struct 
replay_opts *opts)
const char *me = action_name(opts);
int cmd = 0;
struct option base_options[] = {
-   OPT_CMDMODE(0, "quit", , N_("end revert or cherry-pick 
sequence"), 'q'),
-   OPT_CMDMODE(0, "continue", , N_("resume revert or 
cherry-pick sequence"), 'c'),
-   OPT_CMDMODE(0, "abort", , N_("cancel revert or cherry-pick 
sequence"), 'a'),
+   OPT_CMDMODE_F(0, "quit", ,
+ N_("end revert or cherry-pick sequence"),
+ 'q', PARSE_OPT_NOCOMPLETE),
+   OPT_CMDMODE_F(0, "continue", ,
+ N_("resume revert or cherry-pick sequence"),
+ 'c', PARSE_OPT_NOCOMPLETE),
+   OPT_CMDMODE_F(0, "abort", ,
+ N_("cancel revert or cherry-pick sequence"),
+ 'a', PARSE_OPT_NOCOMPLETE),
OPT_BOOL('n', "no-commit", >no_commit, N_("don't 
automatically commit")),
OPT_BOOL('e', "edit", >edit, N_("edit the commit 
message")),
OPT_NOOP_NOARG('r', NULL),
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index a830c9c854..d6215c494e 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2687,10 +2687,7 @@ _git_revert ()
fi
case "$cur" in
--*)
-   __gitcomp "
-   --edit --mainline --no-edit --no-commit --signoff
-   --strategy= --strategy-option=
-   "
+   __gitcomp_builtin revert "--no-edit"
return
;;
esac
-- 
2.16.1.205.g271f633410

[PATCH v2 27/41] completion: use __gitcomp_builtin in _git_mv

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

The new completable option is --verbose.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/mv.c   | 3 ++-
 contrib/completion/git-completion.bash | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index b88023a733..e3e308d282 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -122,7 +122,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
struct option builtin_mv_options[] = {
OPT__VERBOSE(, N_("be verbose")),
OPT__DRY_RUN(_only, N_("dry run")),
-   OPT__FORCE(, N_("force move/rename even if target 
exists"), 0),
+   OPT__FORCE(, N_("force move/rename even if target 
exists"),
+  PARSE_OPT_NOCOMPLETE),
OPT_BOOL('k', NULL, _errors, N_("skip move/rename 
errors")),
OPT_END(),
};
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 1bf1728b95..0e36190a19 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1795,7 +1795,7 @@ _git_mv ()
 {
case "$cur" in
--*)
-   __gitcomp "--dry-run"
+   __gitcomp_builtin mv
return
;;
esac
-- 
2.16.1.205.g271f633410

[PATCH v2 34/41] completion: use __gitcomp_builtin in _git_replace

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

The new completable option is --raw.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/replace.c  | 3 ++-
 contrib/completion/git-completion.bash | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index 10078ae371..0c1d8e1b08 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -439,7 +439,8 @@ int cmd_replace(int argc, const char **argv, const char 
*prefix)
OPT_CMDMODE('d', "delete", , N_("delete replace refs"), 
MODE_DELETE),
OPT_CMDMODE('e', "edit", , N_("edit existing object"), 
MODE_EDIT),
OPT_CMDMODE('g', "graft", , N_("change a commit's 
parents"), MODE_GRAFT),
-   OPT_BOOL('f', "force", , N_("replace the ref if it 
exists")),
+   OPT_BOOL_F('f', "force", , N_("replace the ref if it 
exists"),
+  PARSE_OPT_NOCOMPLETE),
OPT_BOOL(0, "raw", , N_("do not pretty-print contents for 
--edit")),
OPT_STRING(0, "format", , N_("format"), N_("use this 
format")),
OPT_END()
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 7431ffc750..2f3f11451d 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2647,7 +2647,7 @@ _git_replace ()
 {
case "$cur" in
--*)
-   __gitcomp "--edit --graft --format= --list --delete"
+   __gitcomp_builtin replace
return
;;
esac
-- 
2.16.1.205.g271f633410

[PATCH v2 35/41] completion: use __gitcomp_builtin in _git_reset

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

The new completable options are:

--intent-to-add
--quiet
--recurse-submodules

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 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 2f3f11451d..a830c9c854 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2671,7 +2671,7 @@ _git_reset ()
 
case "$cur" in
--*)
-   __gitcomp "--merge --mixed --hard --soft --patch --keep"
+   __gitcomp_builtin reset
return
;;
esac
-- 
2.16.1.205.g271f633410

[PATCH v2 32/41] completion: use __gitcomp_builtin in _git_remote

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

No new completable options!

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 contrib/completion/git-completion.bash | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 8233dede74..7431ffc750 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2598,7 +2598,7 @@ _git_remote ()
if [ -z "$subcommand" ]; then
case "$cur" in
--*)
-   __gitcomp "--verbose"
+   __gitcomp_builtin remote
;;
*)
__gitcomp "$subcommands"
@@ -2609,33 +2609,33 @@ _git_remote ()
 
case "$subcommand,$cur" in
add,--*)
-   __gitcomp "--track --master --fetch --tags --no-tags --mirror="
+   __gitcomp_builtin remote_add "--no-tags"
;;
add,*)
;;
set-head,--*)
-   __gitcomp "--auto --delete"
+   __gitcomp_builtin remote_set-head
;;
set-branches,--*)
-   __gitcomp "--add"
+   __gitcomp_builtin remote_set-branches
;;
set-head,*|set-branches,*)
__git_complete_remote_or_refspec
;;
update,--*)
-   __gitcomp "--prune"
+   __gitcomp_builtin remote_update
;;
update,*)
__gitcomp "$(__git_get_config_variables "remotes")"
;;
set-url,--*)
-   __gitcomp "--push --add --delete"
+   __gitcomp_builtin remote_set-url
;;
get-url,--*)
-   __gitcomp "--push --all"
+   __gitcomp_builtin remote_get-url
;;
prune,--*)
-   __gitcomp "--dry-run"
+   __gitcomp_builtin remote_prune
;;
*)
__gitcomp_nl "$(__git_remotes)"
-- 
2.16.1.205.g271f633410

[PATCH v2 23/41] completion: use __gitcomp_builtin in _git_ls_files

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

The new completable options are:

--debug
--empty-directory
--eol
--recurse-submodules
--resolve-undo

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 contrib/completion/git-completion.bash | 8 +---
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 3e0973a562..e65e71760f 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1602,13 +1602,7 @@ _git_ls_files ()
 {
case "$cur" in
--*)
-   __gitcomp "--cached --deleted --modified --others --ignored
-   --stage --directory --no-empty-directory --unmerged
-   --killed --exclude= --exclude-from=
-   --exclude-per-directory= --exclude-standard
-   --error-unmatch --with-tree= --full-name
-   --abbrev --ignored --exclude-per-directory
-   "
+   __gitcomp_builtin ls-files "--no-empty-directory"
return
;;
esac
-- 
2.16.1.205.g271f633410

[PATCH v2 31/41] completion: use __gitcomp_builtin in _git_push

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

The new completable options are:

--atomic
--exec=
--ipv4
--ipv6
--no-verify
--porcelain
--progress
--push-option
--signed

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/push.c | 2 +-
 contrib/completion/git-completion.bash | 7 +--
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 1c28427d82..013c20d616 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -548,7 +548,7 @@ int cmd_push(int argc, const char **argv, const char 
*prefix)
{ OPTION_CALLBACK, 0, "recurse-submodules", 
_submodules, "check|on-demand|no",
N_("control recursive pushing of submodules"),
PARSE_OPT_OPTARG, option_parse_recurse_submodules },
-   OPT_BOOL( 0 , "thin", , N_("use thin pack")),
+   OPT_BOOL_F( 0 , "thin", , N_("use thin pack"), 
PARSE_OPT_NOCOMPLETE),
OPT_STRING( 0 , "receive-pack", , "receive-pack", 
N_("receive pack program")),
OPT_STRING( 0 , "exec", , "receive-pack", 
N_("receive pack program")),
OPT_BIT('u', "set-upstream", , N_("set upstream for git 
pull/status"),
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 225507b751..8233dede74 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1914,12 +1914,7 @@ _git_push ()
return
;;
--*)
-   __gitcomp "
-   --all --mirror --tags --dry-run --force --verbose
-   --quiet --prune --delete --follow-tags
-   --receive-pack= --repo= --set-upstream
-   --force-with-lease --force-with-lease= 
--recurse-submodules=
-   "
+   __gitcomp_builtin push
return
;;
esac
-- 
2.16.1.205.g271f633410

[PATCH v2 22/41] completion: use __gitcomp_builtin in _git_init

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

The new completable option is --separate-git-dir=.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 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 1e3cb39fde..3e0973a562 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1592,7 +1592,7 @@ _git_init ()
return
;;
--*)
-   __gitcomp "--quiet --bare --template= --shared --shared="
+   __gitcomp_builtin init
return
;;
esac
-- 
2.16.1.205.g271f633410

[PATCH v2 29/41] completion: use __gitcomp_builtin in _git_notes

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

The new completable options are:

--allow-empty (notes add and notes append)
--for-rewrite= (notes copy)

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/notes.c|  4 ++--
 contrib/completion/git-completion.bash | 14 --
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index b52e71c73e..6990683bd4 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -413,7 +413,7 @@ static int add(int argc, const char **argv, const char 
*prefix)
parse_reuse_arg},
OPT_BOOL(0, "allow-empty", _empty,
N_("allow storing empty note")),
-   OPT__FORCE(, N_("replace existing notes"), 0),
+   OPT__FORCE(, N_("replace existing notes"), 
PARSE_OPT_NOCOMPLETE),
OPT_END()
};
 
@@ -484,7 +484,7 @@ static int copy(int argc, const char **argv, const char 
*prefix)
struct notes_tree *t;
const char *rewrite_cmd = NULL;
struct option options[] = {
-   OPT__FORCE(, N_("replace existing notes"), 0),
+   OPT__FORCE(, N_("replace existing notes"), 
PARSE_OPT_NOCOMPLETE),
OPT_BOOL(0, "stdin", _stdin, N_("read objects from 
stdin")),
OPT_STRING(0, "for-rewrite", _cmd, N_("command"),
   N_("load rewriting config for  (implies "
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 391074a772..14af9457af 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1821,7 +1821,7 @@ _git_notes ()
 
case "$subcommand,$cur" in
,--*)
-   __gitcomp '--ref'
+   __gitcomp_builtin notes
;;
,*)
case "$prev" in
@@ -1837,15 +1837,17 @@ _git_notes ()
add,--reedit-message=*|append,--reedit-message=*)
__git_complete_refs --cur="${cur#*=}"
;;
-   add,--*|append,--*)
-   __gitcomp '--file= --message= --reedit-message=
-   --reuse-message='
+   add,--*)
+   __gitcomp_builtin notes_add
+   ;;
+   append,--*)
+   __gitcomp_builtin notes_append
;;
copy,--*)
-   __gitcomp '--stdin'
+   __gitcomp_builtin notes_copy
;;
prune,--*)
-   __gitcomp '--dry-run --verbose'
+   __gitcomp_builtin notes_prune
;;
prune,*)
;;
-- 
2.16.1.205.g271f633410

[PATCH v2 30/41] completion: use __gitcomp_builtin in _git_pull

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

This is really nice. Since pull_options[] already declares all
passthru options to 'merge' or 'fetch', a single

git pull --git-completion-helper

would provide all completable options (--no- variants are a separate
issue). Dead shell variables can now be deleted.

New completable options are:

--allow-unrelated-histories
--ipv4
--ipv6
--jobs
--refmap=
--signoff
--strategy-option=

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 contrib/completion/git-completion.bash | 25 +
 1 file changed, 5 insertions(+), 20 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 14af9457af..225507b751 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1434,12 +1434,6 @@ _git_difftool ()
 
 __git_fetch_recurse_submodules="yes on-demand no"
 
-__git_fetch_options="
-   --quiet --verbose --append --upload-pack --force --keep --depth=
-   --tags --no-tags --all --prune --dry-run --recurse-submodules=
-   --unshallow --update-shallow
-"
-
 _git_fetch ()
 {
case "$cur" in
@@ -1740,14 +1734,6 @@ _git_log ()
__git_complete_revlist
 }
 
-# Common merge options shared by git-merge(1) and git-pull(1).
-__git_merge_options="
-   --no-commit --no-stat --log --no-log --squash --strategy
-   --commit --stat --no-squash --ff --no-ff --ff-only --edit --no-edit
-   --verify-signatures --no-verify-signatures --gpg-sign
-   --quiet --verbose --progress --no-progress
-"
-
 _git_merge ()
 {
__git_complete_strategy && return
@@ -1873,12 +1859,11 @@ _git_pull ()
return
;;
--*)
-   __gitcomp "
-   --rebase --no-rebase
-   --autostash --no-autostash
-   $__git_merge_options
-   $__git_fetch_options
-   "
+   __gitcomp_builtin pull "--no-autostash --no-commit --no-edit
+   --no-ff --no-log --no-progress 
--no-rebase
+   --no-squash --no-stat --no-tags
+   --no-verify-signatures"
+
return
;;
esac
-- 
2.16.1.205.g271f633410

[PATCH v2 25/41] completion: use __gitcomp_builtin in _git_merge

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

New completable options are:

--allow-unrelated-histories
--message=
--overwrite-ignore
--signoff
--strategy-option=
--summary
--verify

The variable $__git_merge_options remains because _git_pull() still
needs it. It will soon be gone after _git_pull() is updated.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 contrib/completion/git-completion.bash | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index f1eb37fbff..52baa7869f 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1754,8 +1754,13 @@ _git_merge ()
 
case "$cur" in
--*)
-   __gitcomp "$__git_merge_options
-   --rerere-autoupdate --no-rerere-autoupdate --abort 
--continue"
+   __gitcomp_builtin merge "--rerere-autoupdate
+   --no-rerere-autoupdate
+   --no-commit --no-edit --no-ff
+   --no-log --no-progress
+   --no-squash --no-stat
+   --no-verify-signatures
+   "
return
esac
__git_complete_refs
-- 
2.16.1.205.g271f633410

[PATCH v2 28/41] completion: use __gitcomp_builtin in _git_name_rev

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

The new completable options are:

--always
--exclude
--name-only
--refs
--undefined

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 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 0e36190a19..391074a772 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1811,7 +1811,7 @@ _git_mv ()
 
 _git_name_rev ()
 {
-   __gitcomp "--tags --all --stdin"
+   __gitcomp_builtin name-rev
 }
 
 _git_notes ()
-- 
2.16.1.205.g271f633410

[PATCH v2 26/41] completion: use __gitcomp_builtin in _git_merge_base

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

The new completion option is --all.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 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 52baa7869f..1bf1728b95 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1784,7 +1784,7 @@ _git_merge_base ()
 {
case "$cur" in
--*)
-   __gitcomp "--octopus --independent --is-ancestor --fork-point"
+   __gitcomp_builtin merge-base
return
;;
esac
-- 
2.16.1.205.g271f633410

[PATCH v2 24/41] completion: use __gitcomp_builtin in _git_ls_remote

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

The new completable options are --quiet and --upload-pack=.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/ls-remote.c| 5 +++--
 contrib/completion/git-completion.bash | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index c4be98ab9e..540d56429f 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -60,8 +60,9 @@ 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_SET_INT(0, "exit-code", ,
-   N_("exit with exit code 2 if no matching refs are 
found"), 2),
+   OPT_SET_INT_F(0, "exit-code", ,
+ N_("exit with exit code 2 if no matching refs are 
found"),
+ 2, PARSE_OPT_NOCOMPLETE),
OPT_BOOL(0, "symref", _symref_target,
 N_("show underlying ref in addition to the object 
pointed by it")),
OPT_END()
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index e65e71760f..f1eb37fbff 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1616,7 +1616,7 @@ _git_ls_remote ()
 {
case "$cur" in
--*)
-   __gitcomp "--heads --tags --refs --get-url --symref"
+   __gitcomp_builtin ls-remote
return
;;
esac
-- 
2.16.1.205.g271f633410

[PATCH v2 17/41] completion: use __gitcomp_builtin in _git_fetch

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

New completable options:

--deepen=
--ipv4
--ipv6
--jobs=
--multiple
--progress
--refmap=
--shallow-exclude=
--shallow-since=
--update-head-ok

Since _git_pull() needs fetch options too, $__git_fetch_options
remains. This variable will soon be gone after _git_pull() is updated.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 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 255d60f12d..316dac84e0 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1448,7 +1448,7 @@ _git_fetch ()
return
;;
--*)
-   __gitcomp "$__git_fetch_options"
+   __gitcomp_builtin fetch "--no-tags"
return
;;
esac
-- 
2.16.1.205.g271f633410

[PATCH v2 15/41] completion: use __gitcomp_builtin in _git_describe

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

No new completable options!

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 contrib/completion/git-completion.bash | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 1713a583cf..450d8e488e 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1355,11 +1355,7 @@ _git_describe ()
 {
case "$cur" in
--*)
-   __gitcomp "
-   --all --tags --contains --abbrev= --candidates=
-   --exact-match --debug --long --match --always 
--first-parent
-   --exclude --dirty --broken
-   "
+   __gitcomp_builtin describe
return
esac
__git_complete_refs
-- 
2.16.1.205.g271f633410

[PATCH v2 13/41] completion: use __gitcomp_builtin in _git_commit

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

The new comletable options are:

--branch
--gpg-sign
--long
--no-post-rewrite
--null
--porcelain
--status

--allow-empty is no longer completable because it's a hidden option
since 4741edd549 (Remove deprecated OPTION_BOOLEAN for parsing arguments
- 2013-08-03)

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 contrib/completion/git-completion.bash | 11 +--
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 393f86619d..8bbe94a94f 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1339,16 +1339,7 @@ _git_commit ()
return
;;
--*)
-   __gitcomp "
-   --all --author= --signoff --verify --no-verify
-   --edit --no-edit
-   --amend --include --only --interactive
-   --dry-run --reuse-message= --reedit-message=
-   --reset-author --file= --message= --template=
-   --cleanup= --untracked-files --untracked-files=
-   --verbose --quiet --fixup= --squash=
-   --patch --short --date --allow-empty
-   "
+   __gitcomp_builtin commit "--no-edit --verify"
return
esac
 
-- 
2.16.1.205.g271f633410

[PATCH v2 14/41] completion: use __gitcomp_builtin in _git_config

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

The new completable options are:

--blob=
--bool
--bool-or-int
--edit
--expiry-date
--get-color
--get-colorbool
--get-urlmatch
--includes
--int
--null
--path
--show-origin

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 contrib/completion/git-completion.bash | 9 +
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 8bbe94a94f..1713a583cf 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2238,14 +2238,7 @@ _git_config ()
esac
case "$cur" in
--*)
-   __gitcomp "
-   --system --global --local --file=
-   --list --replace-all
-   --get --get-all --get-regexp
-   --add --unset --unset-all
-   --remove-section --rename-section
-   --name-only
-   "
+   __gitcomp_builtin config
return
;;
branch.*.*)
-- 
2.16.1.205.g271f633410

[PATCH v2 16/41] completion: use __gitcomp_builtin in _git_difftool

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

Since we can't automatically extract diff options for completion yet,
difftool will take all options from $__git_diff_common_options. This
brings _a lot_ more completable options to difftool.

--ignore-submodules is added to $__git_diff_common_options to avoid
regression in difftool. But it's a good thing anyway even for other
diff commands.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 contrib/completion/git-completion.bash | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 450d8e488e..255d60f12d 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1380,7 +1380,7 @@ __git_diff_common_options="--stat --numstat --shortstat 
--summary
--dirstat --dirstat= --dirstat-by-file
--dirstat-by-file= --cumulative
--diff-algorithm=
-   --submodule --submodule=
+   --submodule --submodule= --ignore-submodules
 "
 
 _git_diff ()
@@ -1421,11 +1421,11 @@ _git_difftool ()
return
;;
--*)
-   __gitcomp "--cached --staged --pickaxe-all --pickaxe-regex
-   --base --ours --theirs
-   --no-renames --diff-filter= --find-copies-harder
-   --relative --ignore-submodules
-   --tool="
+   __gitcomp_builtin difftool "$__git_diff_common_options
+   --base --cached --ours --theirs
+   --pickaxe-all --pickaxe-regex
+   --relative --staged
+   "
return
;;
esac
-- 
2.16.1.205.g271f633410

[PATCH v2 19/41] completion: use __gitcomp_builtin in _git_gc

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

The new completable option is --quiet.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/gc.c   | 7 +--
 contrib/completion/git-completion.bash | 2 +-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 3c5eae0edf..7fc5c16254 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -360,8 +360,11 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
N_("prune unreferenced objects"),
PARSE_OPT_OPTARG, NULL, (intptr_t)prune_expire },
OPT_BOOL(0, "aggressive", , N_("be more thorough 
(increased runtime)")),
-   OPT_BOOL(0, "auto", _gc, N_("enable auto-gc mode")),
-   OPT_BOOL(0, "force", , N_("force running gc even if there 
may be another gc running")),
+   OPT_BOOL_F(0, "auto", _gc, N_("enable auto-gc mode"),
+  PARSE_OPT_NOCOMPLETE),
+   OPT_BOOL_F(0, "force", ,
+  N_("force running gc even if there may be another gc 
running"),
+  PARSE_OPT_NOCOMPLETE),
OPT_END()
};
 
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 4d25cfa047..f694331cfc 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1495,7 +1495,7 @@ _git_gc ()
 {
case "$cur" in
--*)
-   __gitcomp "--prune --aggressive"
+   __gitcomp_builtin gc
return
;;
esac
-- 
2.16.1.205.g271f633410

[PATCH v2 21/41] completion: use __gitcomp_builtin in _git_help

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

No new completable options!

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 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 6e78737a13..1e3cb39fde 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1569,7 +1569,7 @@ _git_help ()
 {
case "$cur" in
--*)
-   __gitcomp "--all --guides --info --man --web"
+   __gitcomp_builtin help
return
;;
esac
-- 
2.16.1.205.g271f633410

[PATCH v2 12/41] completion: use __gitcomp_builtin in _git_clone

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

The new completable options are:

--config
--dissociate
--ipv4
--ipv6
--jobs=
--progress
--reference-if-able
--separate-git-dir=
--shallow-exclude
--shallow-since=
--verbose

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 contrib/completion/git-completion.bash | 21 +
 1 file changed, 1 insertion(+), 20 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index c0f6f76c3c..393f86619d 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1306,26 +1306,7 @@ _git_clone ()
 {
case "$cur" in
--*)
-   __gitcomp "
-   --local
-   --no-hardlinks
-   --shared
-   --reference
-   --quiet
-   --no-checkout
-   --bare
-   --mirror
-   --origin
-   --upload-pack
-   --template=
-   --depth
-   --single-branch
-   --no-tags
-   --branch
-   --recurse-submodules
-   --no-single-branch
-   --shallow-submodules
-   "
+   __gitcomp_builtin clone "--no-single-branch"
return
;;
esac
-- 
2.16.1.205.g271f633410

[PATCH v2 18/41] completion: use __gitcomp_builtin in _git_fsck

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

The new completable options are:

--connectivity-only
--dangling
--progress
--reflogs

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 contrib/completion/git-completion.bash | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 316dac84e0..4d25cfa047 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1485,10 +1485,7 @@ _git_fsck ()
 {
case "$cur" in
--*)
-   __gitcomp "
-   --tags --root --unreachable --cache --no-reflogs --full
-   --strict --verbose --lost-found --name-objects
-   "
+   __gitcomp_builtin fsck "--no-reflogs"
return
;;
esac
-- 
2.16.1.205.g271f633410

[PATCH v2 11/41] completion: use __gitcomp_builtin in _git_clean

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

The new completable options are --exclude and --interactive

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/clean.c| 2 +-
 contrib/completion/git-completion.bash | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index 3c4ca9a2ff..fad533a0a7 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -909,7 +909,7 @@ int cmd_clean(int argc, const char **argv, const char 
*prefix)
struct option options[] = {
OPT__QUIET(, N_("do not print names of files removed")),
OPT__DRY_RUN(_run, N_("dry run")),
-   OPT__FORCE(, N_("force"), 0),
+   OPT__FORCE(, N_("force"), PARSE_OPT_NOCOMPLETE),
OPT_BOOL('i', "interactive", , N_("interactive 
cleaning")),
OPT_BOOL('d', NULL, _directories,
N_("remove whole directories")),
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 1b2e510cf6..c0f6f76c3c 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1293,7 +1293,7 @@ _git_clean ()
 {
case "$cur" in
--*)
-   __gitcomp "--dry-run --quiet"
+   __gitcomp_builtin clean
return
;;
esac
-- 
2.16.1.205.g271f633410

[PATCH v2 10/41] completion: use __gitcomp_builtin in _git_cherry_pick

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

The new completable options are:

--allow-empty
--allow-empty-message
--ff
--gpg-sign
--keep-redundant-commits
--strategy-option

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 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 61880275ed..1b2e510cf6 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1281,7 +1281,7 @@ _git_cherry_pick ()
fi
case "$cur" in
--*)
-   __gitcomp "--edit --no-commit --signoff --strategy= --mainline"
+   __gitcomp_builtin cherry-pick
;;
*)
__git_complete_refs
-- 
2.16.1.205.g271f633410

[PATCH v2 09/41] completion: use __gitcomp_builtin in _git_checkout

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

The new completable options are:

--ignore-other-worktrees
--progress

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/checkout.c |  7 +--
 contrib/completion/git-completion.bash |  6 +-
 t/t9902-completion.sh  | 12 +++-
 3 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2a96358eb7..a6218024a6 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1118,9 +1118,12 @@ int cmd_checkout(int argc, const char **argv, const char 
*prefix)
2),
OPT_SET_INT('3', "theirs", _stage, N_("checkout 
their version for unmerged files"),
3),
-   OPT__FORCE(, N_("force checkout (throw away local 
modifications)"), 0),
+   OPT__FORCE(, N_("force checkout (throw away local 
modifications)"),
+  PARSE_OPT_NOCOMPLETE),
OPT_BOOL('m', "merge", , N_("perform a 3-way merge 
with the new branch")),
-   OPT_BOOL(0, "overwrite-ignore", _ignore, 
N_("update ignored files (default)")),
+   OPT_BOOL_F(0, "overwrite-ignore", _ignore,
+  N_("update ignored files (default)"),
+  PARSE_OPT_NOCOMPLETE),
OPT_STRING(0, "conflict", _style, N_("style"),
   N_("conflict style (merge or diff3)")),
OPT_BOOL('p', "patch", _mode, N_("select hunks 
interactively")),
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 4a2858f09c..61880275ed 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1252,11 +1252,7 @@ _git_checkout ()
__gitcomp "diff3 merge" "" "${cur##--conflict=}"
;;
--*)
-   __gitcomp "
-   --quiet --ours --theirs --track --no-track --merge
-   --conflict= --orphan --patch --detach 
--ignore-skip-worktree-bits
-   --recurse-submodules --no-recurse-submodules
-   "
+   __gitcomp_builtin checkout "--no-track --no-recurse-submodules"
;;
*)
# check if --track, --no-track, or --no-guess was specified
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index fc614dcbfa..e6485feb0a 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1237,17 +1237,19 @@ test_expect_success 'double dash "git" itself' '
 test_expect_success 'double dash "git checkout"' '
test_completion "git checkout --" <<-\EOF
--quiet Z
+   --detach Z
+   --track Z
+   --orphan=Z
--ours Z
--theirs Z
-   --track Z
-   --no-track Z
--merge Z
-   --conflict=
-   --orphan Z
+   --conflict=Z
--patch Z
-   --detach Z
--ignore-skip-worktree-bits Z
+   --ignore-other-worktrees Z
--recurse-submodules Z
+   --progress Z
+   --no-track Z
--no-recurse-submodules Z
EOF
 '
-- 
2.16.1.205.g271f633410

[PATCH v2 08/41] completion: use __gitcomp_builtin in _git_branch

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

The new completable options are:

--all
--create-reflog
--format=
--ignore-case
--quiet

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/branch.c   | 2 +-
 contrib/completion/git-completion.bash | 8 ++--
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index b5b62c08ba..6d0cea9d4b 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -615,7 +615,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
OPT_BOOL('l', "create-reflog", , N_("create the branch's 
reflog")),
OPT_BOOL(0, "edit-description", _description,
 N_("edit the description for the branch")),
-   OPT__FORCE(, N_("force creation, move/rename, deletion"), 
0),
+   OPT__FORCE(, N_("force creation, move/rename, deletion"), 
PARSE_OPT_NOCOMPLETE),
OPT_MERGED(, N_("print only branches that are merged")),
OPT_NO_MERGED(, N_("print only branches that are not 
merged")),
OPT_COLUMN(0, "column", , N_("list branches in 
columns")),
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 6f763c524e..4a2858f09c 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1209,12 +1209,8 @@ _git_branch ()
__git_complete_refs --cur="${cur##--set-upstream-to=}"
;;
--*)
-   __gitcomp "
-   --color --no-color --verbose --abbrev= --no-abbrev
-   --track --no-track --contains --no-contains --merged 
--no-merged
-   --set-upstream-to= --edit-description --list
-   --unset-upstream --delete --move --copy --remotes
-   --column --no-column --sort= --points-at
+   __gitcomp_builtin branch "--no-color --no-abbrev
+   --no-track --no-column
"
;;
*)
-- 
2.16.1.205.g271f633410

[PATCH v2 02/41] parse-options: add OPT_xxx_F() variants

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

These macros allow us to add extra parse-options flag, the main one in
my mind is PARSE_OPT_NOCOMPLETE to hide certain options from
--git-completion-helper.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 parse-options.h | 25 -
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/parse-options.h b/parse-options.h
index 983de27cf0..bc41b99d3b 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -115,23 +115,30 @@ struct option {
intptr_t defval;
 };
 
+#define OPT_BIT_F(s, l, v, h, b, f) { OPTION_BIT, (s), (l), (v), NULL, (h), \
+ PARSE_OPT_NOARG|(f), NULL, (b) }
+#define OPT_COUNTUP_F(s, l, v, h, f) { OPTION_COUNTUP, (s), (l), (v), NULL, \
+  (h), PARSE_OPT_NOARG|(f) }
+#define OPT_SET_INT_F(s, l, v, h, i, f) { OPTION_SET_INT, (s), (l), (v), NULL, 
\
+ (h), PARSE_OPT_NOARG | (f), NULL, (i) 
}
+#define OPT_BOOL_F(s, l, v, h, f)   OPT_SET_INT_F(s, l, v, h, 1, f)
+#define OPT_CMDMODE_F(s, l, v, h, i, f) { OPTION_CMDMODE, (s), (l), (v), NULL, 
\
+ (h), 
PARSE_OPT_NOARG|PARSE_OPT_NONEG|(f), \
+ NULL, (i) }
+
 #define OPT_END()   { OPTION_END }
 #define OPT_ARGUMENT(l, h)  { OPTION_ARGUMENT, 0, (l), NULL, NULL, \
  (h), PARSE_OPT_NOARG}
 #define OPT_GROUP(h){ OPTION_GROUP, 0, NULL, NULL, NULL, (h) }
-#define OPT_BIT(s, l, v, h, b)  { OPTION_BIT, (s), (l), (v), NULL, (h), \
- PARSE_OPT_NOARG, NULL, (b) }
+#define OPT_BIT(s, l, v, h, b)  OPT_BIT_F(s, l, v, h, b, 0)
 #define OPT_NEGBIT(s, l, v, h, b)   { OPTION_NEGBIT, (s), (l), (v), NULL, \
  (h), PARSE_OPT_NOARG, NULL, (b) }
-#define OPT_COUNTUP(s, l, v, h) { OPTION_COUNTUP, (s), (l), (v), NULL, \
- (h), PARSE_OPT_NOARG }
-#define OPT_SET_INT(s, l, v, h, i)  { OPTION_SET_INT, (s), (l), (v), NULL, \
- (h), PARSE_OPT_NOARG, NULL, (i) }
-#define OPT_BOOL(s, l, v, h)OPT_SET_INT(s, l, v, h, 1)
+#define OPT_COUNTUP(s, l, v, h) OPT_COUNTUP_F(s, l, v, h, 0)
+#define OPT_SET_INT(s, l, v, h, i)  OPT_SET_INT_F(s, l, v, h, i, 0)
+#define OPT_BOOL(s, l, v, h)OPT_BOOL_F(s, l, v, h, 0)
 #define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \
  (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, 
NULL, 1}
-#define OPT_CMDMODE(s, l, v, h, i)  { OPTION_CMDMODE, (s), (l), (v), NULL, \
- (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, 
NULL, (i) }
+#define OPT_CMDMODE(s, l, v, h, i)  OPT_CMDMODE_F(s, l, v, h, i, 0)
 #define OPT_INTEGER(s, l, v, h) { OPTION_INTEGER, (s), (l), (v), N_("n"), 
(h) }
 #define OPT_MAGNITUDE(s, l, v, h)   { OPTION_MAGNITUDE, (s), (l), (v), \
  N_("n"), (h), PARSE_OPT_NONEG }
-- 
2.16.1.205.g271f633410

[PATCH v2 03/41] parse-options: let OPT__FORCE take optional flags argument

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

--force option is most likely hidden from command line completion for
safety reasons. This is done by adding an extra flag
PARSE_OPT_NOCOMPLETE. Update OPT__FORCE() to accept additional
flags. Actual flag change comes later depending on individual
commands.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/add.c| 2 +-
 builtin/branch.c | 2 +-
 builtin/checkout-index.c | 2 +-
 builtin/checkout.c   | 2 +-
 builtin/clean.c  | 2 +-
 builtin/fetch.c  | 2 +-
 builtin/mv.c | 2 +-
 builtin/notes.c  | 4 ++--
 builtin/pull.c   | 2 +-
 builtin/rm.c | 2 +-
 builtin/tag.c| 2 +-
 builtin/update-server-info.c | 2 +-
 builtin/worktree.c   | 4 +++-
 parse-options.h  | 2 +-
 14 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index bf01d89e28..ac7c1c3277 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -294,7 +294,7 @@ static struct option builtin_add_options[] = {
OPT_BOOL('i', "interactive", _interactive, N_("interactive 
picking")),
OPT_BOOL('p', "patch", _interactive, N_("select hunks 
interactively")),
OPT_BOOL('e', "edit", _interactive, N_("edit current diff and 
apply")),
-   OPT__FORCE(_too, N_("allow adding otherwise ignored files")),
+   OPT__FORCE(_too, N_("allow adding otherwise ignored files"), 0),
OPT_BOOL('u', "update", _worktree_changes, N_("update tracked 
files")),
OPT_BOOL(0, "renormalize", _renormalize, N_("renormalize EOL of 
tracked files (implies -u)")),
OPT_BOOL('N', "intent-to-add", _to_add, N_("record only the fact 
that the path will be added later")),
diff --git a/builtin/branch.c b/builtin/branch.c
index 8dcc2ed058..b5b62c08ba 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -615,7 +615,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
OPT_BOOL('l', "create-reflog", , N_("create the branch's 
reflog")),
OPT_BOOL(0, "edit-description", _description,
 N_("edit the description for the branch")),
-   OPT__FORCE(, N_("force creation, move/rename, deletion")),
+   OPT__FORCE(, N_("force creation, move/rename, deletion"), 
0),
OPT_MERGED(, N_("print only branches that are merged")),
OPT_NO_MERGED(, N_("print only branches that are not 
merged")),
OPT_COLUMN(0, "column", , N_("list branches in 
columns")),
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index b0e78b819d..a730f6a1aa 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -157,7 +157,7 @@ int cmd_checkout_index(int argc, const char **argv, const 
char *prefix)
struct option builtin_checkout_index_options[] = {
OPT_BOOL('a', "all", ,
N_("check out all files in the index")),
-   OPT__FORCE(, N_("force overwrite of existing files")),
+   OPT__FORCE(, N_("force overwrite of existing files"), 0),
OPT__QUIET(,
N_("no warning for existing files and files not in 
index")),
OPT_BOOL('n', "no-create", _new,
diff --git a/builtin/checkout.c b/builtin/checkout.c
index c54c78df54..2a96358eb7 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1118,7 +1118,7 @@ int cmd_checkout(int argc, const char **argv, const char 
*prefix)
2),
OPT_SET_INT('3', "theirs", _stage, N_("checkout 
their version for unmerged files"),
3),
-   OPT__FORCE(, N_("force checkout (throw away local 
modifications)")),
+   OPT__FORCE(, N_("force checkout (throw away local 
modifications)"), 0),
OPT_BOOL('m', "merge", , N_("perform a 3-way merge 
with the new branch")),
OPT_BOOL(0, "overwrite-ignore", _ignore, 
N_("update ignored files (default)")),
OPT_STRING(0, "conflict", _style, N_("style"),
diff --git a/builtin/clean.c b/builtin/clean.c
index 189e20628c..3c4ca9a2ff 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -909,7 +909,7 @@ int cmd_clean(int argc, const char **argv, const char 
*prefix)
struct option options[] = {
OPT__QUIET(, N_("do not print names of files removed")),
OPT__DRY_RUN(_run, N_("dry run")),
-   OPT__FORCE(, N_("force")),
+   OPT__FORCE(, N_("force"), 0),
OPT_BOOL('i', "interactive", , N_("interactive 
cleaning")),
OPT_BOOL('d', NULL, _directories,
N_("remove whole directories")),
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 7bbcd26faf..6a603174fb 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -115,7 +115,7 @@ static struct option builtin_fetch_options[] = {
 

[PATCH v2 00/41] Automate updating git-completion.bash a bit

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

I posted a proof of concept a while back [1]. This is the full version.

This series lets "git" binary help git-completion.bash to complete
-- so that when a new option is added, we don't have to update
git-completion.bash manually too (people often forget it). As a side
effect, about 180 more options are now completable.

parse-options is updated to allow developers to flag certain options
not to be completable if they want finer control over it.  But by
default, new non-hidden options are completable. Negative forms must
be handled manually. That's for the next step.

The number of patches is high, but changes after the first four
patches and 33/41 are quite simple. I still need some eyeballs though
to make sure I have not accidentally allowed completion of dangerous
options. Details are broken down per command in each commit message.

If people want to play with this, I have a script [2] that shows all
completable options for most commands (I ignore some that are
shell-based because I don't touch them in this series). You can then
do a "diff" to see new/old options.

There's a small conflict with 'pu' because --prune-tags is added in
git-completion.bash. The solution is simple and beautiful: ignore
those changes, --prune-tags will be completable anyway :)

[1] https://public-inbox.org/git/%3c20180116103700.4505-1-pclo...@gmail.com%3E/
[2] https://gist.github.com/pclouds/f337d4393b5cfab813909b8eea2eaa40

Nguyễn Thái Ngọc Duy (41):
  parse-options: support --git-completion-helper
  parse-options: add OPT_xxx_F() variants
  parse-options: let OPT__FORCE take optional flags argument
  git-completion.bash: introduce __gitcomp_builtin
  completion: use __gitcomp_builtin in _git_add
  completion: use __gitcomp_builtin in _git_am
  completion: use __gitcomp_builtin in _git_apply
  completion: use __gitcomp_builtin in _git_branch
  completion: use __gitcomp_builtin in _git_checkout
  completion: use __gitcomp_builtin in _git_cherry_pick
  completion: use __gitcomp_builtin in _git_clean
  completion: use __gitcomp_builtin in _git_clone
  completion: use __gitcomp_builtin in _git_commit
  completion: use __gitcomp_builtin in _git_config
  completion: use __gitcomp_builtin in _git_describe
  completion: use __gitcomp_builtin in _git_difftool
  completion: use __gitcomp_builtin in _git_fetch
  completion: use __gitcomp_builtin in _git_fsck
  completion: use __gitcomp_builtin in _git_gc
  completion: use __gitcomp_builtin in _git_grep
  completion: use __gitcomp_builtin in _git_help
  completion: use __gitcomp_builtin in _git_init
  completion: use __gitcomp_builtin in _git_ls_files
  completion: use __gitcomp_builtin in _git_ls_remote
  completion: use __gitcomp_builtin in _git_merge
  completion: use __gitcomp_builtin in _git_merge_base
  completion: use __gitcomp_builtin in _git_mv
  completion: use __gitcomp_builtin in _git_name_rev
  completion: use __gitcomp_builtin in _git_notes
  completion: use __gitcomp_builtin in _git_pull
  completion: use __gitcomp_builtin in _git_push
  completion: use __gitcomp_builtin in _git_remote
  remote: force completing --mirror= instead of --mirror
  completion: use __gitcomp_builtin in _git_replace
  completion: use __gitcomp_builtin in _git_reset
  completion: use __gitcomp_builtin in _git_revert
  completion: use __gitcomp_builtin in _git_rm
  completion: use __gitcomp_builtin in _git_show_branch
  completion: use __gitcomp_builtin in _git_status
  completion: use __gitcomp_builtin in _git_tag
  completion: use __gitcomp_builtin in _git_worktree

 apply.c|   5 +-
 builtin/add.c  |   2 +-
 builtin/am.c   |  16 +-
 builtin/branch.c   |   2 +-
 builtin/checkout-index.c   |   2 +-
 builtin/checkout.c |   7 +-
 builtin/clean.c|   2 +-
 builtin/fetch.c|   2 +-
 builtin/gc.c   |   7 +-
 builtin/grep.c |  13 +-
 builtin/ls-remote.c|   5 +-
 builtin/mv.c   |   3 +-
 builtin/notes.c|   4 +-
 builtin/pull.c |   2 +-
 builtin/push.c |   2 +-
 builtin/remote.c   |   2 +-
 builtin/replace.c  |   3 +-
 builtin/revert.c   |  12 +-
 builtin/rm.c   |   2 +-
 builtin/tag.c  |   2 +-
 builtin/update-server-info.c   |   2 +-
 builtin/worktree.c |   4 +-
 contrib/completion/git-completion.bash | 276 +
 parse-options.c|  46 +
 parse-options.h|  40 ++--
 rerere.h   |   3 +-
 t/t9902-completion.sh  |  12 +-
 27 files changed, 236 insertions(+), 242 deletions(-)

-- 
2.16.1.205.g271f633410

[PATCH v2 06/41] completion: use __gitcomp_builtin in _git_am

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

The new completable options are:

--directory
--exclude
--gpg-sign
--include
--keep-cr
--keep-non-patch
--message-id
--no-keep-cr
--patch-format
--quiet
--reject
--resolvemsg=

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/am.c   | 16 
 contrib/completion/git-completion.bash |  7 +--
 parse-options.h|  4 ++--
 rerere.h   |  3 ++-
 4 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index acfe9d3c8c..d42832dc0b 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2237,18 +2237,18 @@ int cmd_am(int argc, const char **argv, const char 
*prefix)
PARSE_OPT_NOARG),
OPT_STRING(0, "resolvemsg", , NULL,
N_("override error message when patch failure occurs")),
-   OPT_CMDMODE(0, "continue", ,
+   OPT_CMDMODE_F(0, "continue", ,
N_("continue applying patches after resolving a 
conflict"),
-   RESUME_RESOLVED),
-   OPT_CMDMODE('r', "resolved", ,
+   RESUME_RESOLVED, PARSE_OPT_NOCOMPLETE),
+   OPT_CMDMODE_F('r', "resolved", ,
N_("synonyms for --continue"),
-   RESUME_RESOLVED),
-   OPT_CMDMODE(0, "skip", ,
+   RESUME_RESOLVED, PARSE_OPT_NOCOMPLETE),
+   OPT_CMDMODE_F(0, "skip", ,
N_("skip the current patch"),
-   RESUME_SKIP),
-   OPT_CMDMODE(0, "abort", ,
+   RESUME_SKIP, PARSE_OPT_NOCOMPLETE),
+   OPT_CMDMODE_F(0, "abort", ,
N_("restore the original branch and abort the patching 
operation."),
-   RESUME_ABORT),
+   RESUME_ABORT, PARSE_OPT_NOCOMPLETE),
OPT_BOOL(0, "committer-date-is-author-date",
_date_is_author_date,
N_("lie about committer date")),
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 9aa3aa241b..ce53b78c81 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1110,12 +1110,7 @@ _git_am ()
return
;;
--*)
-   __gitcomp "
-   --3way --committer-date-is-author-date --ignore-date
-   --ignore-whitespace --ignore-space-change
-   --interactive --keep --no-utf8 --signoff --utf8
-   --whitespace= --scissors
-   "
+   __gitcomp_builtin am "--no-utf8"
return
esac
 }
diff --git a/parse-options.h b/parse-options.h
index 23c912b842..fa75df17b4 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -146,8 +146,8 @@ struct option {
 #define OPT_STRING_LIST(s, l, v, a, h) \
{ OPTION_CALLBACK, (s), (l), (v), (a), \
  (h), 0, _opt_string_list }
-#define OPT_UYN(s, l, v, h) { OPTION_CALLBACK, (s), (l), (v), NULL, \
- (h), PARSE_OPT_NOARG, _opt_tertiary 
}
+#define OPT_UYN(s, l, v, h, f)  { OPTION_CALLBACK, (s), (l), (v), NULL, \
+ (h), PARSE_OPT_NOARG|(f), 
_opt_tertiary }
 #define OPT_DATE(s, l, v, h) \
{ OPTION_CALLBACK, (s), (l), (v), N_("time"),(h), 0,\
  parse_opt_approxidate_cb }
diff --git a/rerere.h b/rerere.h
index c2961feaaa..5e5a312e4c 100644
--- a/rerere.h
+++ b/rerere.h
@@ -37,6 +37,7 @@ extern void rerere_clear(struct string_list *);
 extern void rerere_gc(struct string_list *);
 
 #define OPT_RERERE_AUTOUPDATE(v) OPT_UYN(0, "rerere-autoupdate", (v), \
-   N_("update the index with reused conflict resolution if possible"))
+   N_("update the index with reused conflict resolution if possible"), \
+   PARSE_OPT_NOCOMPLETE)
 
 #endif
-- 
2.16.1.205.g271f633410

[PATCH v2 04/41] git-completion.bash: introduce __gitcomp_builtin

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

This is a __gitcomp wrapper that will execute

git ... --git-completion-helper

to get the list of completable options. The call will be made only
once and cached to avoid performance issues, especially on Windows.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 contrib/completion/git-completion.bash | 24 
 1 file changed, 24 insertions(+)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 3683c772c5..2d8d3434c6 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -280,6 +280,30 @@ __gitcomp ()
esac
 }
 
+# This function is equivalent to
+#
+#__gitcomp "$(git xxx --git-completion-helper) ..."
+#
+# except that the value from $(git) is cached
+__gitcomp_builtin ()
+{
+   # spaces must be replaced with underscore for multi-word
+   # commands, e.g. "git remote add" becomes remote_add.
+   local cmd="$1"
+   shift
+
+   local var=__gitcomp_builtin_"${cmd/-/_}"
+   local options
+   eval "options=\$$var"
+
+   if [ -z "$options" ]; then
+   declare -g "$var=$(__git ${cmd/_/ } --git-completion-helper)"
+   eval "options=\$$var"
+   fi
+
+   __gitcomp "$options $*"
+}
+
 # Variation of __gitcomp_nl () that appends to the existing list of
 # completion candidates, COMPREPLY.
 __gitcomp_nl_append ()
-- 
2.16.1.205.g271f633410

[PATCH v2 05/41] completion: use __gitcomp_builtin in _git_add

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

The new completable options are

--all
--ignore-missing
--ignore-removal
--renormalize
--verbose

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 contrib/completion/git-completion.bash | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 2d8d3434c6..9aa3aa241b 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1144,10 +1144,7 @@ _git_add ()
 {
case "$cur" in
--*)
-   __gitcomp "
-   --interactive --refresh --patch --update --dry-run
-   --ignore-errors --intent-to-add --force --edit --chmod=
-   "
+   __gitcomp_builtin add
return
esac
 
-- 
2.16.1.205.g271f633410

[PATCH v2 07/41] completion: use __gitcomp_builtin in _git_apply

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

The new completable options are:

--3way
--allow-overlap
--build-fake-ancestor=
--directory
--exclude
--include

--index-info is no longer completable but that's because it's renamed to
--build-fake-ancestor in 26b2800768 (apply: get rid of --index-info in
favor of --build-fake-ancestor - 2007-09-17)

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 apply.c| 5 +++--
 contrib/completion/git-completion.bash | 9 +
 2 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/apply.c b/apply.c
index 321a9fa68d..65eb37e6a4 100644
--- a/apply.c
+++ b/apply.c
@@ -4943,8 +4943,9 @@ int apply_parse_options(int argc, const char **argv,
N_("make sure the patch is applicable to the current 
index")),
OPT_BOOL(0, "cached", >cached,
N_("apply a patch without touching the working tree")),
-   OPT_BOOL(0, "unsafe-paths", >unsafe_paths,
-   N_("accept a patch that touches outside the working 
area")),
+   OPT_BOOL_F(0, "unsafe-paths", >unsafe_paths,
+  N_("accept a patch that touches outside the working 
area"),
+  PARSE_OPT_NOCOMPLETE),
OPT_BOOL(0, "apply", force_apply,
N_("also apply the patch (use with 
--stat/--summary/--check)")),
OPT_BOOL('3', "3way", >threeway,
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index ce53b78c81..6f763c524e 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1123,14 +1123,7 @@ _git_apply ()
return
;;
--*)
-   __gitcomp "
-   --stat --numstat --summary --check --index
-   --cached --index-info --reverse --reject --unidiff-zero
-   --apply --no-add --exclude=
-   --ignore-whitespace --ignore-space-change
-   --whitespace= --inaccurate-eof --verbose
-   --recount --directory=
-   "
+   __gitcomp_builtin apply
return
esac
 }
-- 
2.16.1.205.g271f633410

[PATCH v2 01/41] parse-options: support --git-completion-helper

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

This option is designed to be used by git-completion.bash. For many
simple cases, what we do in there is usually

__gitcomp "lots of completion options"

which has to be manually updated when a new user-visible option is
added. With support from parse-options, we can write

__gitcomp "$(git command --git-completion-helper)"

and get that list directly from the parser for free. Dangerous/Unpopular
options could be hidden with the new "NOCOMPLETE" flag.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 parse-options.c | 44 
 parse-options.h |  5 -
 2 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/parse-options.c b/parse-options.c
index fca7159646..29f4defdd6 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -425,6 +425,46 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
parse_options_check(options);
 }
 
+/*
+ * TODO: we are not completing the --no-XXX form yet because there are
+ * many options that do not suppress it properly.
+ */
+static int show_gitcomp(struct parse_opt_ctx_t *ctx,
+   const struct option *opts)
+{
+   for (; opts->type != OPTION_END; opts++) {
+   const char *suffix = "";
+
+   if (!opts->long_name)
+   continue;
+   if (opts->flags & (PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE))
+   continue;
+
+   switch (opts->type) {
+   case OPTION_GROUP:
+   continue;
+   case OPTION_STRING:
+   case OPTION_FILENAME:
+   case OPTION_INTEGER:
+   case OPTION_MAGNITUDE:
+   case OPTION_CALLBACK:
+   if (opts->flags & PARSE_OPT_NOARG)
+   break;
+   if (opts->flags & PARSE_OPT_OPTARG)
+   break;
+   if (opts->flags & PARSE_OPT_LASTARG_DEFAULT)
+   break;
+   suffix = "=";
+   break;
+   default:
+   break;
+   }
+   printf(" --%s%s", opts->long_name, suffix);
+   }
+   fputc('\n', stdout);
+   exit(0);
+}
+
 static int usage_with_options_internal(struct parse_opt_ctx_t *,
   const char * const *,
   const struct option *, int, int);
@@ -455,6 +495,10 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
if (internal_help && ctx->total == 1 && !strcmp(arg + 1, "h"))
goto show_usage;
 
+   /* lone --git-completion-helper is asked by git-completion.bash 
*/
+   if (ctx->total == 1 && !strcmp(arg + 1, 
"-git-completion-helper"))
+   return show_gitcomp(ctx, options);
+
if (arg[1] != '-') {
ctx->opt = arg + 1;
switch (parse_short_opt(ctx, options)) {
diff --git a/parse-options.h b/parse-options.h
index af711227ae..983de27cf0 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -38,7 +38,8 @@ enum parse_opt_option_flags {
PARSE_OPT_LASTARG_DEFAULT = 16,
PARSE_OPT_NODASH = 32,
PARSE_OPT_LITERAL_ARGHELP = 64,
-   PARSE_OPT_SHELL_EVAL = 256
+   PARSE_OPT_SHELL_EVAL = 256,
+   PARSE_OPT_NOCOMPLETE = 512
 };
 
 struct option;
@@ -89,6 +90,8 @@ typedef int parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
  *   PARSE_OPT_LITERAL_ARGHELP: says that argh shouldn't be enclosed in 
brackets
  * (i.e. '') in the help message.
  * Useful for options with multiple parameters.
+ *   PARSE_OPT_NOCOMPLETE: by default all visible options are completable
+ *git-completion.bash. This option suppresses that.
  *
  * `callback`::
  *   pointer to the callback to use for OPTION_CALLBACK or
-- 
2.16.1.205.g271f633410

Re: Some rough edges of core.fsmonitor

[Duy Nguyen]

On Tue, Jan 30, 2018 at 8:21 AM, Ben Peart  wrote:
>
>
> On 1/28/2018 5:28 PM, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Sun, Jan 28, 2018 at 9:44 PM, Johannes Schindelin
>>  wrote:
>>>
>>> Hi,
>>>
>>> On Sat, 27 Jan 2018, Ævar Arnfjörð Bjarmason wrote:
>>>
 I just got around to testing this since it landed, for context some
 previous poking of mine in [1].

 Issues / stuff I've noticed:

 1) We end up invalidating the untracked cache because stuff in .git/
 changed. For example:

  01:09:24.975524 fsmonitor.c:173 fsmonitor process
 '.git/hooks/fsmonitor-watchman' returned success
  01:09:24.975548 fsmonitor.c:138 fsmonitor_refresh_callback
 '.git'
  01:09:24.975556 fsmonitor.c:138 fsmonitor_refresh_callback
 '.git/config'
  01:09:24.975568 fsmonitor.c:138 fsmonitor_refresh_callback
 '.git/index'
  01:09:25.122726 fsmonitor.c:91  write fsmonitor extension
 successful

 Am I missing something or should we do something like:

  diff --git a/fsmonitor.c b/fsmonitor.c
  index 0af7c4edba..5067b89bda 100644
  --- a/fsmonitor.c
  +++ b/fsmonitor.c
  @@ -118,7 +118,12 @@ static int query_fsmonitor(int version,
 uint64_t last_update, struct strbuf *que

   static void fsmonitor_refresh_callback(struct index_state *istate,
 const char *name)
   {
  -   int pos = index_name_pos(istate, name, strlen(name));
  +   int pos;
  +
  +   if (!strcmp(name, ".git") || starts_with(name, ".git/"))
  +   return;
  +
  +   pos = index_name_pos(istate, name, strlen(name));
>>>
>>>
>>> I would much rather have the fsmonitor hook already exclude those.
>>
>>
>> As documented the fsmonitor-watchman hook we ship doesn't work as
>> described in githooks(5), unless "files in the working directory" is
>> taken to include .git/, but I haven't seen that ever used.
>>
>> On the other hand relying on arbitrary user-provided hooks to do the
>> right thing at the cost of silent performance degradation is bad. If
>> we're going to expect the hook to remove these we should probably
>> warn/die here if it does send us .git/* files.
>>
>
> I'm not sure how often something is modified in the git directory when
> nothing was modified in the working directory but this seems like a nice
> optimization.
>
> We can't just blindly ignore changes under ".git" as the git directory may
> have been moved somewhere else.  Instead we'd need to use get_git_dir().

In theory. But we do blindly ignore changes under ".git" in some
cases, see treat_path() in dir.c for example.

> Rather than assuming the hook will optimize for this particular case, I
> think a better solution would be to update untracked_cache_invalidate_path()
> so that it doesn't invalidate the untracked cache and mark the index as
> dirty when it was asked to invalidate a path under GIT_DIR.  I can't think
> of a case when that would be the desired behavior.

You see, my only problem with this is tying the check with $GIT_DIR,
which may involve normalizing the path and all that stuff because the
current code base is not prepared to deal with that. Simply ignoring
anything in ".git" may work too. Not pretty but it's more in line with
what we have. Though I'm still not sure how it interacts with ".git"
from submodules which is why I still have not sent a patch to update
untracked_cache_invalidate_path() because it does make sense to fix it
in there.

> Somewhat off topic but related to the overall performance discussion: I've
> also thought the untracked cache shouldn't mark the index as dirty except in
> the case where the extension is being added or removed.  We've observed that
> it causes unnecessary index writes that actually slows down overall
> performance.
>
> Since it is a cache, it does not require the index to be written out for
> correctness, it can simply update the cache again the next time it is
> needed. This is typically faster than the cost of the index write so makes
> things faster overall.  I adopted this same model with the fsmonitor
> extension.

If you turn on split index, the write cost should be much much less
(but I think read cost increases a bit due to merging the two indexes
in core; I noticed this but haven't really dug down). You basically
pay writing modified index entries and extensions.

But yeah not writing is possible. The index's dirty flag can show that
only untracked cache extension is dirty, then it's a judgement call
whether to write it down or drop it. You still need to occasionally
write it down though. Dirty directories will fall back to slow path.
If you don't write it down, the set of dirty paths keeps increasing
and will start to slow git-status down. I don't know at what point we
should write it down though if