Re: [bug] assertion in 2.8.4 triggering on old-ish worktree

[Chris Packham]

On Thu, Jun 16, 2016 at 4:59 PM, Chris Packham  wrote:
> Hi All,
>
> I have the git-sh-prompt configured in my .bashrc today I visited an
> old worktree that I haven't really touched in a few years (sorry can't
> remember the git version I was using back then). I received the
> following output when changing to the directory
>
> git: pathspec.c:317: prefix_pathspec: Assertion `item->nowildcard_len
> <= item->len && item->prefix <= item->len' failed.
>
> I assume it's one of the git invocations in git-sh-prompt that's
> hitting the assertion. Any thoughts on what might be triggering it?
> Any debug I can gather?

A bit more info. The directory in question is a uninitialised
submodule. It doesn't trigger in the root of the parent project.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[bug] assertion in 2.8.4 triggering on old-ish worktree

[Chris Packham]

Hi All,

I have the git-sh-prompt configured in my .bashrc today I visited an
old worktree that I haven't really touched in a few years (sorry can't
remember the git version I was using back then). I received the
following output when changing to the directory

git: pathspec.c:317: prefix_pathspec: Assertion `item->nowildcard_len
<= item->len && item->prefix <= item->len' failed.

I assume it's one of the git invocations in git-sh-prompt that's
hitting the assertion. Any thoughts on what might be triggering it?
Any debug I can gather?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[no subject]

[岸洋介]

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

[PATCH 2/2] archive-tar: write extended headers for far-future mtime

[Jeff King]

The ustar format represents timestamps as seconds since the
epoch, but only has room to store 11 octal digits.  To
express anything larger, we need to use an extended header.
This is exactly the same case we fixed for the size field in
the previous commit, and the solution here follows the same
pattern.

This is even mentioned as an issue in f2f0267 (archive-tar:
use xsnprintf for trivial formatting, 2015-09-24), but since
it only affected things far in the future, it wasn't worth
dealing with. But note that my calculations claiming
thousands of years were off there; because our xsnprintf
produces a NUL byte, we only have until the year 2242 to
fix this.

Given that this is just around the corner (geologically
speaking), and because the fix for "size" makes doing this
on top trivial, let's just make it work.

Note that we don't (and never did) handle negative
timestamps (i.e., before 1970). This would probably not be
too hard to support in the same way, but since git does not
support negative timestamps at all, I didn't bother here.

Unlike the last patch for "size", this one is easy to test
efficiently (the magic date is octal 010001):

  $ echo content >file
  $ git add file
  $ GIT_COMMITTER_DATE='@68719476737 +' \
git commit -q -m 'tempori parendum'
  $ git archive HEAD | tar tvf -
  -rw-rw-r-- root/root 8 4147-08-20 03:32 file

With the current tip of master, this dies in xsnprintf (and
prior to f2f0267, it overflowed into the checksum field, and
the resulting tarfile claimed to be from the year 2242).

However, I decided not to include this in the test suite,
because I suspect it will create portability headaches,
including:

  1. The exact format of the system tar's "t" output.

  2. System tars that cannot handle pax headers.

  3. System tars tha cannot handle dates that far in the
 future.

  4. If we replace "tar t" with "tar x", then filesystems
 that cannot handle dates that far in the future.

In other words, we do not really have a reliable tar reader
for these corner cases, so the best we could do is a
byte-for-byte comparison of the output.

Signed-off-by: Jeff King 
---
 archive-tar.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/archive-tar.c b/archive-tar.c
index 7340b64..749722f 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -185,6 +185,14 @@ static inline unsigned long ustar_size(uintmax_t size)
return 0;
 }
 
+static inline unsigned long ustar_mtime(time_t mtime)
+{
+   if (mtime < 0777)
+   return mtime;
+   else
+   return 0;
+}
+
 static void prepare_header(struct archiver_args *args,
   struct ustar_header *header,
   unsigned int mode, unsigned long size)
@@ -192,7 +200,8 @@ static void prepare_header(struct archiver_args *args,
xsnprintf(header->mode, sizeof(header->mode), "%07o", mode & 0);
xsnprintf(header->size, sizeof(header->size), "%011lo",
  S_ISREG(mode) ? ustar_size(size) : 0);
-   xsnprintf(header->mtime, sizeof(header->mtime), "%011lo", (unsigned 
long) args->time);
+   xsnprintf(header->mtime, sizeof(header->mtime), "%011lo",
+ ustar_mtime(args->time));
 
xsnprintf(header->uid, sizeof(header->uid), "%07o", 0);
xsnprintf(header->gid, sizeof(header->gid), "%07o", 0);
@@ -292,6 +301,8 @@ static int write_tar_entry(struct archiver_args *args,
 
if (ustar_size(size) != size)
strbuf_append_ext_header_uint(_header, "size", size);
+   if (ustar_mtime(args->time) != args->time)
+   strbuf_append_ext_header_uint(_header, "mtime", args->time);
 
prepare_header(args, , mode, size);
 
-- 
2.9.0.150.g8bd4cf6
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] archive-tar: write extended headers for file sizes >= 8GB

[Jeff King]

The ustar format has a fixed-length field for the size of
each file entry which is supposed to contain up to 11 bytes
of octal-formatted data plus a NUL or space terminator.

These means that the largest size we can represent is
0777, or 1 byte short of 8GB. The correct solution
for a larger file, according to POSIX.1-2001, is to add an
extended pax header, similar to how we handle long
filenames. This patch does that, and writes zero for the
size field in the ustar header (the last bit is not
mentioned by POSIX, but it matches how GNU tar behaves with
--format=pax).

This should be a strict improvement over the current
behavior, which is to die in xsnprintf with a "BUG".
However, there's some interesting history here.

Prior to f2f0267 (archive-tar: use xsnprintf for trivial
formatting, 2015-09-24), we silently overflowed the "size"
field. The extra bytes ended up in the "mtime" field of the
header, which was then immediately written itself,
overwriting our extra bytes. What that means depends on how
many bytes we wrote.

If the size was 64GB or greater, then we actually overflowed
digits into the mtime field, meaning our value was was
effectively right-shifted by those lost octal digits. And
this patch is again a strict improvement over that.

But if the size was between 8GB and 64GB, then our 12-byte
field held all of the actual digits, and only our NUL
terminator overflowed. According to POSIX, there should be a
NUL or space at the end of the field. However, GNU tar seems
to be lenient here, and will correctly parse a size up 64GB
(minus one) from the field. So sizes in this range might
have just worked, depending on the implementation reading
the tarfile.

This patch is mostly still an improvement there, as the 8GB
limit is specifically mentioned in POSIX as the correct
limit. But it's possible that it could be a regression
(versus the pre-f2f0267 state) if all of the following are
true:

  1. You have a file between 8GB and 64GB.

  2. Your tar implementation _doesn't_ know about pax
 extended headers.

  3. Your tar implementation _does_ parse 12-byte sizes from
 the ustar header without a delimiter.

It's probably not worth worrying about such an obscure set
of conditions, but I'm documenting it here just in case.

There's no test included here. I did confirm that this works
(using GNU tar) with:

  $ dd if=/dev/zero seek=64G bs=1 count=1 of=huge
  $ git add huge
  $ git commit -q -m foo
  $ git archive HEAD | head -c 1 | tar tvf - 2>/dev/null
  -rw-rw-r-- root/root 68719476737 2016-06-15 21:07 huge

Pre-f2f0267, this would yield a bogus size of 8GB, and
post-f2f0267, git-archive simply dies.

Unfortunately, it's quite an expensive test to run. For one
thing, unless your filesystem supports files with holes, it
takes 64GB of disk space (you might think piping straight to
`hash-object --stdin` would be better, but it's not; that
tries to buffer all 64GB in RAM!). Furthermore, hashing and
compressing the object takes several minutes of CPU time.

We could ship just the resulting compressed object data as a
loose object, but even that takes 64MB. So sadly, this code
path remains untested in the test suite.

Signed-off-by: Jeff King 
---
 archive-tar.c | 28 +++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/archive-tar.c b/archive-tar.c
index cb99df2..7340b64 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -137,6 +137,20 @@ static void strbuf_append_ext_header(struct strbuf *sb, 
const char *keyword,
strbuf_addch(sb, '\n');
 }
 
+/*
+ * Like strbuf_append_ext_header, but for numeric values.
+ */
+static void strbuf_append_ext_header_uint(struct strbuf *sb,
+ const char *keyword,
+ uintmax_t value)
+{
+   char buf[40]; /* big enough for 2^128 in decimal, plus NUL */
+   int len;
+
+   len = xsnprintf(buf, sizeof(buf), "%"PRIuMAX, value);
+   strbuf_append_ext_header(sb, keyword, buf, len);
+}
+
 static unsigned int ustar_header_chksum(const struct ustar_header *header)
 {
const unsigned char *p = (const unsigned char *)header;
@@ -163,12 +177,21 @@ static size_t get_path_prefix(const char *path, size_t 
pathlen, size_t maxlen)
return i;
 }
 
+static inline unsigned long ustar_size(uintmax_t size)
+{
+   if (size < 0777UL)
+   return size;
+   else
+   return 0;
+}
+
 static void prepare_header(struct archiver_args *args,
   struct ustar_header *header,
   unsigned int mode, unsigned long size)
 {
xsnprintf(header->mode, sizeof(header->mode), "%07o", mode & 0);
-   xsnprintf(header->size, sizeof(header->size), "%011lo", S_ISREG(mode) ? 
size : 0);
+   xsnprintf(header->size, sizeof(header->size), "%011lo",
+ S_ISREG(mode) ? ustar_size(size) : 0);
xsnprintf(header->mtime, 

[PATCH 0/2] friendlier handling of overflows in archive-tar

[Jeff King]

The ustar format has some fixed-length numeric fields, and it's possible
to generate a git tree that can't be represented (namely file size and
mtime). Since f2f0267 (archive-tar: use xsnprintf for trivial
formatting, 2015-09-24), we detect and die() in these cases. But we can
actually do the friendly (and POSIX-approved) thing, and add extended
pax headers to represent the correct values.

  [1/2]: archive-tar: write extended headers for file sizes >= 8GB
  [2/2]: archive-tar: write extended headers for far-future mtime

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

Re: What's cooking in git.git (Jun 2016, #04; Tue, 14)

[Mike Hommey]

On Wed, Jun 15, 2016 at 11:32:39AM -0700, Junio C Hamano wrote:
> Mike Hommey  writes:
> 
> > On Tue, Jun 14, 2016 at 03:08:04PM -0700, Junio C Hamano wrote:
> 
> >> * mh/connect (2016-06-06) 10 commits
> >>  - connect: [host:port] is legacy for ssh
> >> ...
> >>  - connect: document why we sometimes call get_port after get_host_and_port
> >> 
> >>  Ok, folks, is everybody happy with this version?
> >
> > $gmane/296609
> > $gmane/296610
> 
> Oh, I have seen these, and I know you two are happy.
> 
> But I am having a hard time coming up with a few-line summary for
> this topic.  I can write the beginning part, i.e. "Git-URL parsing
> routine has been rewritten", but the concluding part of the sentence
> cannot be "... has been rewritten for no good reason." if I were to
> mark the topic as "Will merge to 'next'".  The best I can come up
> with is "... has been rewritten (hopefully) without changing the
> benaviour.", but that is not a strong-enough justificaiton to make
> the change to the codebase, either.
> 
> In short, while the update may not introduce new bugs, why would we
> want to have this change in the first place?

My original motivation was to avoid having to copy code from connect.c
into git-cinnabar, which is what I'm currently doing[1]. Things derailed
a little, and we got ourselves somewhat in the middle of a refactor,
that I'm willing to push a little further (like, refactor things such
that host_end only happens once). My hope is that this makes the code
more maintainable.

Mike

1. https://github.com/glandium/git-cinnabar/blob/master/connect.c
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 16/38] resolve_gitlink_ref(): implement using resolve_ref_recursively()

[Michael Haggerty]

On 06/14/2016 07:03 AM, Eric Sunshine wrote:
> On Fri, Jun 3, 2016 at 5:03 PM, Michael Haggerty  wrote:
>> resolve_ref_recursively() can handle references in arbitrary files
>> reference stores, so use it to resolve "gitlink" (i.e., submodule)
>> references. Aside from removing redundant code, this allows submodule
>> lookups to benefit from the much more robust code that we use for
>> reading non-submodule references. And, since the code is now agnostic
>> about reference backends, it will work for any future references
>> backend (so move its definition to refs.c).
>>
>> Signed-off-by: Michael Haggerty 
>> ---
>> diff --git a/refs.c b/refs.c
>> @@ -1299,6 +1299,30 @@ const char *resolve_ref_unsafe(const char *refname, 
>> int resolve_flags,
>> +int resolve_gitlink_ref(const char *path, const char *refname, unsigned 
>> char *sha1)
>> +{
>> +   int len = strlen(path);
>> +   struct strbuf submodule = STRBUF_INIT;
>> +   struct ref_store *refs;
>> +   int flags;
>> +
>> +   while (len && path[len-1] == '/')
>> +   len--;
>> +   if (!len)
>> +   return -1;
>> +
>> +   strbuf_add(, path, len);
> 
> It took me a moment to figure out that you're using the strbuf only
> for its side-effect of giving you a NUL-terminated string needed by
> get_ref_store(), and not because you need any fancy functionality of
> strbuf. I wonder if xstrndup() would have made this clearer.
> 
> Not worth a re-roll.

I agree both that xstrndup() (or, in this case, xmemdupz()) would have
been clearer, and also that it's not worth a re-roll. I'll keep it in
mind if I touch this code again.

Thanks for your comment.

Michael

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

Re: Easiest way to clone over an existing directory?

[brian m. carlson]

On Wed, Jun 15, 2016 at 08:51:34AM -0700, Josh Triplett wrote:
> Currently, every time I set up a new system, I run the following:
> 
> git clone $MY_HOMEDIR
> mv home/.git .
> rm -r home
> git checkout -f
> 
> This seems like an odd dance to go through.  But I can't just git clone
> into ~ directly, because git clone will not clone into an existing
> non-empty directory.
> 
> (I could use "git clone -n" to avoid the unnecessary checkout, but the
> files are small, and it wouldn't remove the need to rmdir so the number
> of commands would remain the same.)
> 
> Does some better way exist to handle this?  And if not, would it make
> sense for git clone to have an option to clone into an existing
> directory (which should also avoid setting junk_work_tree)?

My typical technique is something like the following:

  git init
  git remote add origin https://git.crustytoothpaste.net/git/bmc/homedir.git
  git pull origin master

I'm not sure if that's the officially sanctioned way to do it, but it
does work reliably.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH v4 6/6] send-email: add option --cite to quote the message body

[Tom Russello]

Le 09/06/2016 à 13:49, Matthieu Moy a écrit :
> Samuel GROOT  writes:
> 
>> If used with `in-reply-to=`, cite the message body of the given
>> email file. Otherwise, do nothing.
> 
> It should at least warn when --in-reply-to= is not given
> (either no --in-reply-to or --in-reply-to=). I don't see any
> use-case where a user would want --cite on the command-line and not want
> --in-reply-to=. OTOH, it seems a plausible user-error, and
> the user would appreciate a message saying what's going on.

You're right. We'll warn the user with the next version.

>> @@ -56,6 +57,8 @@ git send-email --dump-aliases
>>  --subject * Email "Subject:"
>>  --in-reply-to * Email "In-Reply-To:"
>>  --in-reply-to* Populate header fields appropriately.
>> +--cite * Quote the message body in the cover if
>> + --compose is set, else in the first 
>> patch.
>>  --[no-]xmailer * Add "X-Mailer:" header (default).
>>  --[no-]annotate* Review each patch that will be sent in 
>> an editor.
>>  --compose  * Open an editor for introduction.
> 
> Just wondering: would it make sense to activate --cite by default when
> --in-reply-to=file is used, and to allow --no-cite to disable this?
> This is something we can easily do now without breaking backward
> compatibility (--in-reply-to=file doesn't exist yet), but would be more
> painful to do later.

Indeed, it can be more intuitive especially for a user who is used to
cite messages.

>> @@ -640,6 +644,7 @@ if (@files) {
>>  usage();
>>  }
>>  
>> +my $message_cited;
> 
> Nit: I read "$message_cited" as "Boolean saying whether the message was
> cited". $cited_message would be clearer to me (but this is to be taken
> with a grain of salt as I'm not a native speaker), since the variable
> holds the content of the cited message.

Sorry, French habits.. :-)

>> +sub do_insert_cited_message {
>> +my $tmp_file = shift;
>> +my $original_file = shift;
>> +
>> +open my $c, "<", $original_file
>> +or die "Failed to open $original_file: " . $!;
>> +
>> +open my $c2, ">", $tmp_file
>> +or die "Failed to open $tmp_file: " . $!;
>> +
>> +# Insertion after the triple-dash
>> +while (<$c>) {
>> +print $c2 $_;
>> +last if (/^---$/);
>> +}
>> +print $c2 $message_cited;
> 
> I would add a newline here to get a blank line between the message cited
> and the diffstat.

Agreed.

> I think non-ascii characters would deserve particular attention here
> too. For example, if the patch contain only ascii and the cited part
> contains UTF-8, does the generated patch have a proper Content-type:
> header?
> 
> I can imagine worse, like a patch containing latin1 character and a
> cited message with another 8-bit encoding.

I tried to manage them with the built-in Base64 module but there is
still work in progress.

>> +test_expect_success $PREREQ 'correct cited message with --in-reply-to and 
>> --compose' '
>> +grep "> On Sat, 12 Jun 2010 15:53:58 +0200, aut...@example.com wrote:" 
>> msgtxt3 &&
> 
> I would prefer to have the full address including the real name here (A
> ) in this example. Actually, after a quick look at
> the code, I don't understand where the name has gone (what's shown here
> is extracted from the From: header).

Agreed, I'll figure out where the problem is.

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

"git-rebase -i --autostash" will leave dangling stash when editor is aborted

[Daniel Hahler]

TEST CASE:

1. Modify a file that need to get stashed on rebasing
2. Run "EDITOR=vim git rebase -i --autostash"
3. Abort with ":cq", which will make Vim exit non-zero

Git then will create an autostash, but aborts with "Could not execute
editor", via the following code in git-rebase--interactive.sh, and does
not restore the autostash - it does not show up with the regular
stashes, like when there is a conflict during the rebase:

git_sequence_editor "$todo" ||
die_abort "Could not execute editor"

Step 2 and 3 and probably merged into a single one for a test case, but
that's how I keep triggering it.

Instead of adding it to the list of stashes, it should probably get
restored/re-applied right away, since no rebase actions have been triggered.


Please CC me in replies.


Cheers,
Daniel.

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

Re: [PATCH] verify-tag: allow to verify signed blob objects

[Jacob Keller]

On Wed, Jun 15, 2016 at 12:24 PM, Junio C Hamano  wrote:
> Michael J Gruber  writes:
>
>>> Or even
>>>
>>>  if ((flags & GPG_VERIFY_BLOB) && (type != OBJ_BLOB))
>>>  "you told me to check blob but didn't give me one";
>>>  } else if (type != OBJ_TAG)
>>>  "you didn't give me a tag";
>>>
>>
>> I just tried to stay as close to the original as possible, but I don't
>> care either way. Your latter version is more strict and would require a
>> slight documentation change, but would be fine, too.
>
> Actually, the message you reused is not reusable for this new mode.
> I guess starting from more strict (which makes sense, as you do not
> want to silently say "Yeah, the blob verifies OK" when the user
> tells you "I want you to verify this blob, and here it is" and hands
> you a tag.  If that were an acceptable behaviour, you do not even
> need VERIFY_BLOB as an option, do you?
>
> So I do not care too strongly about this feature, if it were to be
> added, I think you would need to separate error messages and type
> verification should not be lax, I would think.
>

I agree that Junio's suggestion is (a) both easier to read and (b)
more clear to the end user, and thus preferable.

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

Re: [PATCH v2 5/6] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C

[Eric Sunshine]

On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva  wrote:
> Reimplement `is_expected_rev` & `check_expected_revs` shell function in
> C and add a `--check-expected-revs` subcommand to `git bisect--helper` to
> call it from git-bisect.sh .
> [...]
> Signed-off-by: Pranit Bauva 
> ---
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> @@ -162,13 +162,44 @@ static int bisect_reset(const char *commit)
> +static int is_expected_rev(const char *expected_hex)
> +{
> +   struct strbuf actual_hex = STRBUF_INIT;
> +   int res;
> +
> +   if (strbuf_read_file(_hex, git_path_bisect_expected_rev(), 0) 
> < 0) {
> +   strbuf_release(_hex);
> +   return 0;
> +   }
> +
> +   strbuf_trim(_hex);
> +   res = !strcmp(actual_hex.buf, expected_hex);
> +   strbuf_release(_hex);
> +   return res;
> +}

Not worth a re-roll, but this could be re-structured to avoid having
to remember to release the strbuf at all exits:

struct strbuf actual_hex = ...;
int res = 0;

if (strbuf_read_file(...) >= 0) {
strbuf_trim(...);
res = !strcmp(...);
}
strbuf_release(...);
return res;

Alternately:

if (strbuf_read_file(...) < 0)
goto done;

strbuf_trim(...);
res = !strcmp(...);

done:
strbuf_release(...);
return res;

which is a bit less compact.

> +static int check_expected_revs(const char **revs, int rev_nr)
> +{
> +   int i;
> +
> +   for (i = 0; i < rev_nr; i++) {
> +   if (!is_expected_rev(revs[i])) {
> +   remove_path(git_path_bisect_ancestors_ok());
> +   remove_path(git_path_bisect_expected_rev());
> +   return 0;
> +   }
> +   }
> +   return 0;
> +}

Hmm, all execution paths return 0, so it feels a bit pointless to have
this function return a value at all.

You could also use a 'break' inside the loop rather than 'return'
since the return value is the same inside or outside the loop and
nothing else happens after the loop.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/6] bisect--helper: `bisect_reset` shell function in C

[Eric Sunshine]

On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva  wrote:
> Reimplement `bisect_reset` shell function in C and add a `--bisect-reset`
> subcommand to `git bisect--helper` to call it from git-bisect.sh .
> [...]
> Signed-off-by: Pranit Bauva 
> ---
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> @@ -123,12 +127,48 @@ static int bisect_clean_state(void)
> +static int bisect_reset(const char *commit)
> +{
> +   struct strbuf branch = STRBUF_INIT;
> +
> +   if (!commit) {
> +   if (strbuf_read_file(, git_path_bisect_start(), 0) < 
> 1) {
> +   printf("We are not bisecting.\n");
> +   return 0;
> +   }
> +   strbuf_rtrim();
> +

Style: unnecessary blank line

> +   } else {
> +   struct object_id oid;
> +   if (get_oid(commit, ))
> +   return error(_("'%s' is not a valid commit"), commit);
> +   strbuf_addf(, "%s", commit);

strbuf_addstr(, commit);

> +   }
> +
> +   if (!file_exists(git_path_bisect_head())) {
> +   struct argv_array argv = ARGV_ARRAY_INIT;
> +   argv_array_pushl(, "checkout", branch.buf, "--", NULL);
> +   if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
> +   error(_("Could not check out original HEAD '%s'. Try"
> +   "'git bisect reset '."), 
> branch.buf);
> +   strbuf_release();
> +   argv_array_clear();
> +   return -1;
> +   }
> +   argv_array_clear();
> +   }
> +
> +   strbuf_release();
> +   return bisect_clean_state();
> +}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Document the 'svn propset' command.

[Joseph Pecoraro]

> On Jun 15, 2016, at 1:24 PM, Alfred Perlstein  wrote:
> 
>> On Jun 15, 2016, at 1:21 PM, Junio C Hamano  wrote:
>> 
>> Eric Wong  writes:
>> 
>>> Thanks Alfred,
>>> 
>>> I've removed the '.' from the commit subject, signed-off,
>>> and pushed to my repo for Junio:
>>> 
>>> The following changes since commit 05219a1276341e72d8082d76b7f5ed394b7437a4:
>>> 
>>> Git 2.9 (2016-06-13 10:42:13 -0700)
>>> 
>>> are available in the git repository at:
>>> 
>>> git://bogomips.org/git-svn.git svn-propset-doc
>>> 
>>> for you to fetch changes up to f3961b2eba8ba6aa2fddc827ddf5c26b41391872:
>>> 
>>> Document the 'svn propset' command (2016-06-15 20:11:22 +)
>> 
>> I actually queued it directly on top of v2.3.0-rc0~32^2 (git-svn:
>> support for git-svn propset, 2014-12-07) so that it could go to
>> older maintenance tracks.
>> 
>> I will pick up your Reviewed-by: and redo it.  Thanks.
>> 
> 
> Thank you, always great working with the git project!  
> 
> -Alfred 

Thanks for addressing this!

- Joe

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

Re: [PATCH] Document the 'svn propset' command.

[Alfred Perlstein]

> On Jun 15, 2016, at 1:21 PM, Junio C Hamano  wrote:
> 
> Eric Wong  writes:
> 
>> Thanks Alfred,
>> 
>> I've removed the '.' from the commit subject, signed-off,
>> and pushed to my repo for Junio:
>> 
>> The following changes since commit 05219a1276341e72d8082d76b7f5ed394b7437a4:
>> 
>>  Git 2.9 (2016-06-13 10:42:13 -0700)
>> 
>> are available in the git repository at:
>> 
>>  git://bogomips.org/git-svn.git svn-propset-doc
>> 
>> for you to fetch changes up to f3961b2eba8ba6aa2fddc827ddf5c26b41391872:
>> 
>>  Document the 'svn propset' command (2016-06-15 20:11:22 +)
> 
> I actually queued it directly on top of v2.3.0-rc0~32^2 (git-svn:
> support for git-svn propset, 2014-12-07) so that it could go to
> older maintenance tracks.
> 
> I will pick up your Reviewed-by: and redo it.  Thanks.
> 

Thank you, always great working with the git project!  

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

Re: [PATCH] Document the 'svn propset' command.

[Eric Wong]

Alfred Perlstein  wrote:
> Add example usage to the git-svn documentation.
> 
> Reported-by: Joseph Pecoraro 
> Signed-off-by: Alfred Perlstein  
> ---
> 
> Junio, Pranit, + all,
> 
> A week ago I was requested to provide documentation for the
> 'svn propset' command.  I have attached a diff off of the
> 'maint' branch for this, however it seems to apply cleanly
> to 'master' as well.
> 
> Thank you for your patience.

Thanks Alfred,

I've removed the '.' from the commit subject, signed-off,
and pushed to my repo for Junio:

The following changes since commit 05219a1276341e72d8082d76b7f5ed394b7437a4:

  Git 2.9 (2016-06-13 10:42:13 -0700)

are available in the git repository at:

  git://bogomips.org/git-svn.git svn-propset-doc

for you to fetch changes up to f3961b2eba8ba6aa2fddc827ddf5c26b41391872:

  Document the 'svn propset' command (2016-06-15 20:11:22 +)


Alfred Perlstein (1):
  Document the 'svn propset' command

 Documentation/git-svn.txt | 14 ++
 1 file changed, 14 insertions(+)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/6] bisect--helper: `bisect_clean_state` shell function in C

[Eric Sunshine]

On Wed, Jun 15, 2016 at 2:47 PM, Pranit Bauva  wrote:
> On Wed, Jun 15, 2016 at 11:34 PM, Eric Sunshine  
> wrote:
>> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva  
>> wrote:
>>> Reimplement `bisect_clean_state` shell function in C and add a
>>> `bisect-clean-state` subcommand to `git bisect--helper` to call it from
>>> git-bisect.sh .
>>> [...]
>>> Signed-off-by: Pranit Bauva 
>>> ---
>>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>>> +static int mark_for_removal(const char *refname, const struct object_id 
>>> *oid,
>>> +   int flag, void *cb_data)
>>> +{
>>> +   struct string_list *refs = cb_data;
>>> +   char *ref = xstrfmt("refs/bisect/%s", refname);
>>> +   string_list_append(refs, ref);
>>> +   return 0;
>>> +}
>>> +
>>> +static int bisect_clean_state(void)
>>> +{
>>> +   int result = 0;
>>> +
>>> +   /* There may be some refs packed during bisection */
>>> +   struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
>>> +   for_each_ref_in("refs/bisect/", mark_for_removal, (void *) 
>>> _for_removal);
>>> +   string_list_append(_for_removal, xstrdup("BISECT_HEAD"));
>>> +   result = delete_refs(_for_removal);
>>> +   string_list_clear(_for_removal, 0);
>>
>> This is leaking all the strings added to 'refs_for_removal', isn't it?
>> Either you need to loop over the items and free the strings manually,
>> or (if it's not too ugly), set 'strdup_strings' before invoking
>> string_list_clear().
>
> I didn't carefully see that in the function string_list_clear() it
> only free()'s the memory if strdup_strings is 1. I think changing
> strdup_strings to 1 would be an easy way out but it would make the
> code very ugly and non-trivial.

I disagree about it making the code "*very* ugly and non-trivial". It
is quite trivial. What I meant by "ugly" was that it may be too
intimate with the implementation of string_list. However, since the
solution is already used in the codebase, it may be acceptable. For
instance, in builtin/fetch.c:

/* All names were strdup()ed or strndup()ed */
list.strdup_strings = 1;
string_list_clear(, 0);

which is exactly the approach I was suggesting. You'll find the same
pattern in builtin/shortlog.c.

> On the other hand, I can initialize
> the string as STRING_LIST_INIT_DUP which will automatically set
> strdup_strings as 1 and then also free the memory of ref at that point
> after the string ref was appended to the list. Personally, I will
> prefer the latter one.

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

Re: [PATCH] Document the 'svn propset' command.

[Junio C Hamano]

Eric Wong  writes:

> Thanks Alfred,
>
> I've removed the '.' from the commit subject, signed-off,
> and pushed to my repo for Junio:
>
> The following changes since commit 05219a1276341e72d8082d76b7f5ed394b7437a4:
>
>   Git 2.9 (2016-06-13 10:42:13 -0700)
>
> are available in the git repository at:
>
>   git://bogomips.org/git-svn.git svn-propset-doc
>
> for you to fetch changes up to f3961b2eba8ba6aa2fddc827ddf5c26b41391872:
>
>   Document the 'svn propset' command (2016-06-15 20:11:22 +)

I actually queued it directly on top of v2.3.0-rc0~32^2 (git-svn:
support for git-svn propset, 2014-12-07) so that it could go to
older maintenance tracks.

I will pick up your Reviewed-by: and redo it.  Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH] push: deny policy to prevent pushes to unwanted remotes.

[Lars Schneider]

>> ...
>> 
> 
> Hello Rémi, thanks you for your input ! I'll make the appropriate changes 
> and send a new version as soon as i can !

Hi Antoine,

do you have an updated version already or is this the one I should look at?
http://article.gmane.org/gmane.comp.version-control.git/296445

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

Re: [PATCH] push: deny policy to prevent pushes to unwanted remotes.

[Lars Schneider]

On 06 Jun 2016, at 16:00, Antoine Queru  
wrote:

> Hello Lars, thanks for your reply. 
>> 
>> 
>>> On 30 May 2016, at 06:45, Antoine Queru
>>>  wrote:
>>> 
>>> Currently, a user wanting to prevent accidental pushes to the wrong remote
>>> has to create a pre-push hook.
>>> The feature offers a configuration to allow users to prevent accidental
>>> pushes
>>> to the wrong remote. The user may define a list of whitelisted remotes, a
>>> list
>>> of blacklisted remotes and a default policy ("allow" or "deny"). A push
>>> is denied if the remote is explicitely blacklisted or if it isn't
>>> whitelisted and the default policy is "deny".
>>> 
>>> This feature is intended as a safety net more than a real security, the
>>> user
>>> will always be able to modify the config if he wants to. It is here for him
>>> to
>>> consciously restrict his push possibilities. For example, it may be useful
>>> for an unexperimented user fearing to push to the wrong remote, or for
>>> companies wanting to avoid unintentionnal leaking of private code on public
>>> repositories.
>> 
>> Thanks for working on this feature. Unfortunately I won't be able to test and
>> review it before June 14. I am traveling without laptop and only very
>> sporadic internet access :)
>> 
>> One thing that I noticed already: I think a custom warning/error message for
>> rejected pushes would be important because, as you wrote above, this feature
>> does not provide real security. That means if a push is rejected for someone
>> in an organization then the user needs to understand what is going on. E.g.
>> in my organization I would point the user to the open source contribution
>> guidelines etc.
>> 
>> Thanks,
>> Lars
> 
> I might not understand what you've said, but I think this feature is already 
> implemented in our version, with remote.pushDenyMessage. Is this what you're
> talking about ?

You are right. I was skimming the diff on a very small screen and 
missed that for some reason. Sorry!

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

Re: [PATCH] verify-tag: allow to verify signed blob objects

[Junio C Hamano]

Michael J Gruber  writes:

>> Or even
>> 
>>  if ((flags & GPG_VERIFY_BLOB) && (type != OBJ_BLOB))
>>  "you told me to check blob but didn't give me one";
>>  } else if (type != OBJ_TAG)
>>  "you didn't give me a tag";
>> 
>
> I just tried to stay as close to the original as possible, but I don't
> care either way. Your latter version is more strict and would require a
> slight documentation change, but would be fine, too.

Actually, the message you reused is not reusable for this new mode.
I guess starting from more strict (which makes sense, as you do not
want to silently say "Yeah, the blob verifies OK" when the user
tells you "I want you to verify this blob, and here it is" and hands
you a tag.  If that were an acceptable behaviour, you do not even
need VERIFY_BLOB as an option, do you?

So I do not care too strongly about this feature, if it were to be
added, I think you would need to separate error messages and type
verification should not be lax, I would think.

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

Re: [PATCH] verify-tag: allow to verify signed blob objects

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 15.06.2016 20:39:
> Michael J Gruber  writes:
> 
>> diff --git a/tag.c b/tag.c
>> index d1dcd18..d5f090b 100644
>> --- a/tag.c
>> +++ b/tag.c
>> @@ -39,7 +39,7 @@ int gpg_verify_tag(const unsigned char *sha1, const char 
>> *name_to_report,
>>  int ret;
>>  
>>  type = sha1_object_info(sha1, NULL);
>> -if (type != OBJ_TAG)
>> +if ((type != OBJ_TAG) && ((type != OBJ_BLOB) || !(flags & 
>> GPG_VERIFY_BLOB)))
>>  return error("%s: cannot verify a non-tag object of type %s.",
>>  name_to_report ?
>>  name_to_report :
> 
> The double negation is very hard to read.  I wonder
> 
>   if ((type != OBJ_TAG) &&
> !((type == OBJ_BLOB) && (flags & GPG_VERIFY_BLOB)))
> 
> is easier to follow?  "It is not a tag object, and it's not like we
> were asked to verify blob and the user gave us a blob, either, so
> let's complain" is easier to follow, at least for me.

As a further exercise in boolean algebra, you can pull out the negation
completely:

if (!( (type == OBJ_TAG) || ((type == OBJ_BLOB) && (flags &
GPG_VERIFY_BLOB)) ))

> Or even
> 
>   if ((flags & GPG_VERIFY_BLOB) && (type != OBJ_BLOB))
>   "you told me to check blob but didn't give me one";
>   } else if (type != OBJ_TAG)
>   "you didn't give me a tag";
> 

I just tried to stay as close to the original as possible, but I don't
care either way. Your latter version is more strict and would require a
slight documentation change, but would be fine, too.

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

Re: [PATCH v2 1/6] bisect--helper: `bisect_clean_state` shell function in C

[Pranit Bauva]

Hey Eric,

On Wed, Jun 15, 2016 at 11:34 PM, Eric Sunshine  wrote:
> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva  wrote:
>> Reimplement `bisect_clean_state` shell function in C and add a
>> `bisect-clean-state` subcommand to `git bisect--helper` to call it from
>> git-bisect.sh .
>> [...]
>> Signed-off-by: Pranit Bauva 
>> ---
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> +static int mark_for_removal(const char *refname, const struct object_id 
>> *oid,
>> +   int flag, void *cb_data)
>> +{
>> +   struct string_list *refs = cb_data;
>> +   char *ref = xstrfmt("refs/bisect/%s", refname);
>> +   string_list_append(refs, ref);
>> +   return 0;
>> +}
>> +
>> +static int bisect_clean_state(void)
>> +{
>> +   int result = 0;
>> +
>> +   /* There may be some refs packed during bisection */
>> +   struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
>> +   for_each_ref_in("refs/bisect/", mark_for_removal, (void *) 
>> _for_removal);
>> +   string_list_append(_for_removal, xstrdup("BISECT_HEAD"));
>> +   result = delete_refs(_for_removal);
>> +   string_list_clear(_for_removal, 0);
>
> This is leaking all the strings added to 'refs_for_removal', isn't it?
> Either you need to loop over the items and free the strings manually,
> or (if it's not too ugly), set 'strdup_strings' before invoking
> string_list_clear().

I didn't carefully see that in the function string_list_clear() it
only free()'s the memory if strdup_strings is 1. I think changing
strdup_strings to 1 would be an easy way out but it would make the
code very ugly and non-trivial. On the other hand, I can initialize
the string as STRING_LIST_INIT_DUP which will automatically set
strdup_strings as 1 and then also free the memory of ref at that point
after the string ref was appended to the list. Personally, I will
prefer the latter one.

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

Re: What's cooking in git.git (Jun 2016, #04; Tue, 14)

[Junio C Hamano]

Duy Nguyen  writes:

>> * nd/worktree-cleanup-post-head-protection (2016-05-24) 6 commits
>>  - worktree: simplify prefixing paths
>>  - worktree: avoid 0{40}, too many zeroes, hard to read
>>  - worktree.c: use is_dot_or_dotdot()
>>  - git-worktree.txt: keep subcommand listing in alphabetical order
>>  - worktree.c: rewrite mark_current_worktree() to avoid strbuf
>>  - completion: support git-worktree
>>  (this branch is used by nd/worktree-lock.)
>>
>>  Further preparatory clean-up for "worktree" feature.
>>
>>  Expecting a reroll.
>>  ($gmane/294136, etc.)
>
> Hmm.. I think what's in 'pu' (which is v2, $gmane/295260) is ok now.

Yes, thanks for spotting.  I should have updated the status text
when I picked up the reroll.

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

Re: [PATCH v2 3/6] wrapper: move is_empty_file() from builtin/am.c

[Pranit Bauva]

Hey Eric,

On Wed, Jun 15, 2016 at 11:52 PM, Eric Sunshine  wrote:
> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva  wrote:
>> is_empty_file() can help to refactor a lot of code. Also it is quite
>> helpful while converting shell scripts which use `test -s`. Since
>
> As justification, "can help to refactor a lot of code" is very
> nebulous. It would be better to give a concrete reason for moving the
> function, such as explaining that the functionality will be needed by
> the "git bisect" port to C.

Sure I can include that.

>> is_empty_file() is now a "library" function, its inappropriate to die() so
>> instead error_errno() is used to convey the message to stderr while the
>> appropriate boolean value is returned.
>>
>> Signed-off-by: Pranit Bauva 
>> ---
>> diff --git a/builtin/am.c b/builtin/am.c
>> @@ -30,22 +30,6 @@
>>  /**
>> - * Returns 1 if the file is empty or does not exist, 0 otherwise.
>> - */
>> -static int is_empty_file(const char *filename)
>> -{
>> -   struct stat st;
>> -
>> -   if (stat(filename, ) < 0) {
>> -   if (errno == ENOENT)
>> -   return 1;
>> -   die_errno(_("could not stat %s"), filename);
>> -   }
>> -
>> -   return !st.st_size;
>> -}
>
> So, the original function die()'d for unexpected errors, but the
> rewrite does not. This is a big behavior change. To account for such a
> change in behavior I'd expect to see git-am updated to die() on its
> own for such failures, but no such changes are present in this patch.
> More about this below...
>
>> diff --git a/wrapper.c b/wrapper.c
>> @@ -696,3 +696,16 @@ void sleep_millisec(int millisec)
>> +int is_empty_file(const char *filename)
>> +{
>> +   struct stat st;
>> +
>> +   if (stat(filename, ) < 0) {
>> +   if (errno == ENOENT)
>> +   return 1;
>> +   error_errno(_("could not stat %s"), filename);
>
> Mental note: There is no 'return' in front of error_errno(), so the
> function does not exit here...

This is purposely as I want to keep this function to return only
boolean values ( 0 or 1).

>> +   }
>> +
>> +   return !st.st_size;
>> +}
>
> If stat() returns some error other than ENOENT, then the value of 'st'
> will be undefined, yet this return statement accesses its 'st_size'
> field, which is clearly a bad thing to do.
>
> You either need to return a designated value (such as -1) upon errors
> other than ENOENT (and update the documentation to mention -1) so that
> the caller can decided what to do, or die() as the original did. While
> it's true that die()'ing is not necessarily friendly in library code,
> it may be acceptable until such time that you find a caller which
> needs different behavior.

I didn't realize the complexity the patch carried with itself before.
I probably shouldn't fidget with am code right now, its a work better
left to who are converting the code to library code. I think its the
best fit for this situation to leave it as die()'ing.

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

Re: [PATCH] verify-tag: allow to verify signed blob objects

[Junio C Hamano]

Michael J Gruber  writes:

> diff --git a/tag.c b/tag.c
> index d1dcd18..d5f090b 100644
> --- a/tag.c
> +++ b/tag.c
> @@ -39,7 +39,7 @@ int gpg_verify_tag(const unsigned char *sha1, const char 
> *name_to_report,
>   int ret;
>  
>   type = sha1_object_info(sha1, NULL);
> - if (type != OBJ_TAG)
> + if ((type != OBJ_TAG) && ((type != OBJ_BLOB) || !(flags & 
> GPG_VERIFY_BLOB)))
>   return error("%s: cannot verify a non-tag object of type %s.",
>   name_to_report ?
>   name_to_report :

The double negation is very hard to read.  I wonder

if ((type != OBJ_TAG) &&
!((type == OBJ_BLOB) && (flags & GPG_VERIFY_BLOB)))

is easier to follow?  "It is not a tag object, and it's not like we
were asked to verify blob and the user gave us a blob, either, so
let's complain" is easier to follow, at least for me.

Or even

if ((flags & GPG_VERIFY_BLOB) && (type != OBJ_BLOB))
"you told me to check blob but didn't give me one";
} else if (type != OBJ_TAG)
"you didn't give me a tag";

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

Re: What's cooking in git.git (Jun 2016, #04; Tue, 14)

[Junio C Hamano]

Mike Hommey  writes:

> On Tue, Jun 14, 2016 at 03:08:04PM -0700, Junio C Hamano wrote:

>> * mh/connect (2016-06-06) 10 commits
>>  - connect: [host:port] is legacy for ssh
>> ...
>>  - connect: document why we sometimes call get_port after get_host_and_port
>> 
>>  Ok, folks, is everybody happy with this version?
>
> $gmane/296609
> $gmane/296610

Oh, I have seen these, and I know you two are happy.

But I am having a hard time coming up with a few-line summary for
this topic.  I can write the beginning part, i.e. "Git-URL parsing
routine has been rewritten", but the concluding part of the sentence
cannot be "... has been rewritten for no good reason." if I were to
mark the topic as "Will merge to 'next'".  The best I can come up
with is "... has been rewritten (hopefully) without changing the
benaviour.", but that is not a strong-enough justificaiton to make
the change to the codebase, either.

In short, while the update may not introduce new bugs, why would we
want to have this change in the first place?

By the way, please do not quote the whole thing when you are
responding to a tiny part of the original message.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/6] wrapper: move is_empty_file() from builtin/am.c

[Eric Sunshine]

On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva  wrote:
> is_empty_file() can help to refactor a lot of code. Also it is quite
> helpful while converting shell scripts which use `test -s`. Since

As justification, "can help to refactor a lot of code" is very
nebulous. It would be better to give a concrete reason for moving the
function, such as explaining that the functionality will be needed by
the "git bisect" port to C.

> is_empty_file() is now a "library" function, its inappropriate to die() so
> instead error_errno() is used to convey the message to stderr while the
> appropriate boolean value is returned.
>
> Signed-off-by: Pranit Bauva 
> ---
> diff --git a/builtin/am.c b/builtin/am.c
> @@ -30,22 +30,6 @@
>  /**
> - * Returns 1 if the file is empty or does not exist, 0 otherwise.
> - */
> -static int is_empty_file(const char *filename)
> -{
> -   struct stat st;
> -
> -   if (stat(filename, ) < 0) {
> -   if (errno == ENOENT)
> -   return 1;
> -   die_errno(_("could not stat %s"), filename);
> -   }
> -
> -   return !st.st_size;
> -}

So, the original function die()'d for unexpected errors, but the
rewrite does not. This is a big behavior change. To account for such a
change in behavior I'd expect to see git-am updated to die() on its
own for such failures, but no such changes are present in this patch.
More about this below...

> diff --git a/wrapper.c b/wrapper.c
> @@ -696,3 +696,16 @@ void sleep_millisec(int millisec)
> +int is_empty_file(const char *filename)
> +{
> +   struct stat st;
> +
> +   if (stat(filename, ) < 0) {
> +   if (errno == ENOENT)
> +   return 1;
> +   error_errno(_("could not stat %s"), filename);

Mental note: There is no 'return' in front of error_errno(), so the
function does not exit here...

> +   }
> +
> +   return !st.st_size;
> +}

If stat() returns some error other than ENOENT, then the value of 'st'
will be undefined, yet this return statement accesses its 'st_size'
field, which is clearly a bad thing to do.

You either need to return a designated value (such as -1) upon errors
other than ENOENT (and update the documentation to mention -1) so that
the caller can decided what to do, or die() as the original did. While
it's true that die()'ing is not necessarily friendly in library code,
it may be acceptable until such time that you find a caller which
needs different behavior.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/6] wrapper: move is_empty_file() from builtin/am.c

[Pranit Bauva]

Hey Junio,

On Wed, Jun 15, 2016 at 11:42 PM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>> diff --git a/builtin/am.c b/builtin/am.c
>> index 3dfe70b..84f21d0 100644
>> --- a/builtin/am.c
>> +++ b/builtin/am.c
>> @@ -30,22 +30,6 @@
>>  #include "mailinfo.h"
>>
>>  /**
>> - * Returns 1 if the file is empty or does not exist, 0 otherwise.
>> - */
>> -static int is_empty_file(const char *filename)
>> -{
>> - struct stat st;
>> -
>> - if (stat(filename, ) < 0) {
>> - if (errno == ENOENT)
>> - return 1;
>> - die_errno(_("could not stat %s"), filename);
>> - }
>> -
>> - return !st.st_size;
>> -}
>> -
>
> This is perfectly fine in the context of "git am", but as a public
> function that is called is_empty_file(), callers can come from two
> camps.  One is like the caller of this function in "am" where an
> empty and a missing file are treated equivalently.  The other would
> want to act differently.
>
> Renaming it "is-empty-or-missing" is necessary in order to make it
> clear that this helper function is not targetted for the latter
> callers.

Yes, its better to rename the function as is_empty_or_missing() . Thanks!

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

Re: [PATCH v2 3/6] wrapper: move is_empty_file() from builtin/am.c

[Junio C Hamano]

Pranit Bauva  writes:

> diff --git a/builtin/am.c b/builtin/am.c
> index 3dfe70b..84f21d0 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -30,22 +30,6 @@
>  #include "mailinfo.h"
>  
>  /**
> - * Returns 1 if the file is empty or does not exist, 0 otherwise.
> - */
> -static int is_empty_file(const char *filename)
> -{
> - struct stat st;
> -
> - if (stat(filename, ) < 0) {
> - if (errno == ENOENT)
> - return 1;
> - die_errno(_("could not stat %s"), filename);
> - }
> -
> - return !st.st_size;
> -}
> -

This is perfectly fine in the context of "git am", but as a public
function that is called is_empty_file(), callers can come from two
camps.  One is like the caller of this function in "am" where an
empty and a missing file are treated equivalently.  The other would
want to act differently.

Renaming it "is-empty-or-missing" is necessary in order to make it
clear that this helper function is not targetted for the latter
callers.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/6] convert various shell functions in git-bisect to C

[Pranit Bauva]

Hey Eric,

On Wed, Jun 15, 2016 at 11:23 PM, Eric Sunshine  wrote:
> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva  wrote:
>> Changes wrt previous version:
>>  * Use STRING_LIST_INIT_NODUP to avoid leaks in bisect_clean_state()
>>  * Use test_path_is_missing in the patch 2/6
>>  * drop file_size()
>>  * move is_empty_file() method from builtin/am.c to wrapper.c
>>  * use static for methods
>>  * remove the variable status in bisect_reset() altogether and put the whole
>>thing inside the if block.
>>  * one more method converted namely bisect_write().
>
> As an aid to reviewers, in the future, please also include in the
> cover letter an interdiff between the previous and current version of
> the patch series. Thanks.

Sure!

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

Re: [PATCH v2 1/6] bisect--helper: `bisect_clean_state` shell function in C

[Eric Sunshine]

On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva  wrote:
> Reimplement `bisect_clean_state` shell function in C and add a
> `bisect-clean-state` subcommand to `git bisect--helper` to call it from
> git-bisect.sh .
> [...]
> Signed-off-by: Pranit Bauva 
> ---
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> +static int mark_for_removal(const char *refname, const struct object_id *oid,
> +   int flag, void *cb_data)
> +{
> +   struct string_list *refs = cb_data;
> +   char *ref = xstrfmt("refs/bisect/%s", refname);
> +   string_list_append(refs, ref);
> +   return 0;
> +}
> +
> +static int bisect_clean_state(void)
> +{
> +   int result = 0;
> +
> +   /* There may be some refs packed during bisection */
> +   struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
> +   for_each_ref_in("refs/bisect/", mark_for_removal, (void *) 
> _for_removal);
> +   string_list_append(_for_removal, xstrdup("BISECT_HEAD"));
> +   result = delete_refs(_for_removal);
> +   string_list_clear(_for_removal, 0);

This is leaking all the strings added to 'refs_for_removal', isn't it?
Either you need to loop over the items and free the strings manually,
or (if it's not too ugly), set 'strdup_strings' before invoking
string_list_clear().

> +   remove_path(git_path_bisect_expected_rev());
> +   remove_path(git_path_bisect_ancestors_ok());
> +   remove_path(git_path_bisect_log());
> +   remove_path(git_path_bisect_names());
> +   remove_path(git_path_bisect_run());
> +   remove_path(git_path_bisect_write_terms());
> +   /* Cleanup head-name if it got left by an old version of git-bisect */
> +   remove_path(git_path_head_name());
> +   /*
> +* Cleanup BISECT_START last to support the --no-checkout option
> +* introduced in the commit 4796e823a.
> +*/
> +   remove_path(git_path_bisect_start());
> +
> +   return result;
> +}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] hooks--pre-commit.sample: check for chars, that are not allowed for a windows file name

[Junio C Hamano]

dexteritas  writes:

> After the ASCII-check, test the windows compatibility of file names.
> Can be disabled by:
> git config hooks.allownonwindowschars true
> ---

Missing sign off.

>  templates/hooks--pre-commit.sample | 22 ++
>  1 file changed, 22 insertions(+)
>
> diff --git a/templates/hooks--pre-commit.sample 
> b/templates/hooks--pre-commit.sample
> index 68d62d5..120daf1 100755
> --- a/templates/hooks--pre-commit.sample
> +++ b/templates/hooks--pre-commit.sample
> @@ -17,6 +17,7 @@ fi
>  
>  # If you want to allow non-ASCII filenames set this variable to true.
>  allownonascii=$(git config --bool hooks.allownonascii)
> +allownonwindowschars=$(git config --bool hooks.allownonwindowschars)
>  
>  # Redirect output to stderr.
>  exec 1>&2
> @@ -43,6 +44,27 @@ If you know what you are doing you can disable this check 
> using:
>git config hooks.allownonascii true
>  EOF
>   exit 1
> +elif [ "$allownonwindowschars" != "true" ] &&

This line is doubly irritating because (1) the double negation is
somewhat hard to grok, and (2) [] is not part of our CodingStyle.

Because you inherited this from the existing "allow-non-ascii" one,
however, I do not want to see you change this line, if you need to
reroll.

> + # If you work with linux and windows, there is a problem, if you use
> + # chars like \ / : * ? " < > |

There is no reason to single out Linux, is there?

This new check is only about Windows and because people on other
platforms, not limited to Linux, may not be aware of some characters
that are not usable on Windows, you are trying to help them, no?

> + # Check if there are used only windows compatible chars
> + test $(git diff --cached --name-only --diff-filter=A -z $against |
> +   LC_ALL=C tr -d '[0-9A-Za-z\[\]\{\}_ -)+-.]\0' | wc -c) != 0

Because this is in "elif", we know we are allowing non-ascii
characters when we come here.  So you need to do a quite similar
check from scratch, which is sensible.  I do not offhand know if
this covers all the characters that Windows users cannot use,
though.

> +then
> + cat <<\EOF
> +Error: Attempt to add a chars that are not allowed for a windows file name.
> +
> +This can cause problems if you want to work with people on other platforms.
> +
> +To be portable it is advisable to rename the file.
> +
> +Check your filenames for: \ / : * ? " < > |
> +
> +If you know what you are doing you can disable this check using:
> +
> +  git config hooks.allownonwindowschars true
> +EOF
> + exit 2

Why 2?

>  fi
>  
>  # If there are whitespace errors, print the offending file names and fail.

When the user says

[hooks]
allownonascii = false
allownonwindowschars = false

what happens?

The user's intention clearly is that the project wants to be usable
on Windows and also wants to exclude characters from codepages that
are not ASCII.  I however suspect that the hook with your patch will
allow people to create a "path>like?this.txt" happily.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/6] convert various shell functions in git-bisect to C

[Eric Sunshine]

On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva  wrote:
> Changes wrt previous version:
>  * Use STRING_LIST_INIT_NODUP to avoid leaks in bisect_clean_state()
>  * Use test_path_is_missing in the patch 2/6
>  * drop file_size()
>  * move is_empty_file() method from builtin/am.c to wrapper.c
>  * use static for methods
>  * remove the variable status in bisect_reset() altogether and put the whole
>thing inside the if block.
>  * one more method converted namely bisect_write().

As an aid to reviewers, in the future, please also include in the
cover letter an interdiff between the previous and current version of
the patch series. Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Easiest way to clone over an existing directory?

[Josh Triplett]

Currently, every time I set up a new system, I run the following:

git clone $MY_HOMEDIR
mv home/.git .
rm -r home
git checkout -f

This seems like an odd dance to go through.  But I can't just git clone
into ~ directly, because git clone will not clone into an existing
non-empty directory.

(I could use "git clone -n" to avoid the unnecessary checkout, but the
files are small, and it wouldn't remove the need to rmdir so the number
of commands would remain the same.)

Does some better way exist to handle this?  And if not, would it make
sense for git clone to have an option to clone into an existing
directory (which should also avoid setting junk_work_tree)?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 5/6] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C

[Pranit Bauva]

Reimplement `is_expected_rev` & `check_expected_revs` shell function in
C and add a `--check-expected-revs` subcommand to `git bisect--helper` to
call it from git-bisect.sh .

Using `--check-expected-revs` subcommand is a temporary measure to port
shell functions to C so as to use the existing test suite. As more
functions are ported, this subcommand would be retired and will be
called by some other method.

Helped-by: Eric Sunshine 
Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 37 -
 git-bisect.sh| 20 ++--
 2 files changed, 38 insertions(+), 19 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 14043a8..f11c247 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -162,13 +162,44 @@ static int bisect_reset(const char *commit)
return bisect_clean_state();
 }
 
+static int is_expected_rev(const char *expected_hex)
+{
+   struct strbuf actual_hex = STRBUF_INIT;
+   int res;
+
+   if (strbuf_read_file(_hex, git_path_bisect_expected_rev(), 0) < 
0) {
+   strbuf_release(_hex);
+   return 0;
+   }
+
+   strbuf_trim(_hex);
+   res = !strcmp(actual_hex.buf, expected_hex);
+   strbuf_release(_hex);
+   return res;
+}
+
+static int check_expected_revs(const char **revs, int rev_nr)
+{
+   int i;
+
+   for (i = 0; i < rev_nr; i++) {
+   if (!is_expected_rev(revs[i])) {
+   remove_path(git_path_bisect_ancestors_ok());
+   remove_path(git_path_bisect_expected_rev());
+   return 0;
+   }
+   }
+   return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
NEXT_ALL = 1,
WRITE_TERMS,
BISECT_CLEAN_STATE,
-   BISECT_RESET
+   BISECT_RESET,
+   CHECK_EXPECTED_REVS
} cmdmode = 0;
int no_checkout = 0;
struct option options[] = {
@@ -180,6 +211,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 N_("cleanup the bisection state"), BISECT_CLEAN_STATE),
OPT_CMDMODE(0, "bisect-reset", ,
 N_("reset the bisection state"), BISECT_RESET),
+   OPT_CMDMODE(0, "check-expected-revs", ,
+N_("check for expected revs"), CHECK_EXPECTED_REVS),
OPT_BOOL(0, "no-checkout", _checkout,
 N_("update BISECT_HEAD instead of checking out the 
current commit")),
OPT_END()
@@ -206,6 +239,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
if (argc > 1)
die(_("--bisect-reset requires either zero or one 
arguments"));
return bisect_reset(argc ? argv[0] : NULL);
+   case CHECK_EXPECTED_REVS:
+   return check_expected_revs(argv, argc);
default:
die("BUG: unknown subcommand '%d'", cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index 18580b7..4f6545e 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -238,22 +238,6 @@ bisect_write() {
test -n "$nolog" || echo "git bisect $state $rev" 
>>"$GIT_DIR/BISECT_LOG"
 }
 
-is_expected_rev() {
-   test -f "$GIT_DIR/BISECT_EXPECTED_REV" &&
-   test "$1" = $(cat "$GIT_DIR/BISECT_EXPECTED_REV")
-}
-
-check_expected_revs() {
-   for _rev in "$@"; do
-   if ! is_expected_rev "$_rev"
-   then
-   rm -f "$GIT_DIR/BISECT_ANCESTORS_OK"
-   rm -f "$GIT_DIR/BISECT_EXPECTED_REV"
-   return
-   fi
-   done
-}
-
 bisect_skip() {
all=''
for arg in "$@"
@@ -280,7 +264,7 @@ bisect_state() {
rev=$(git rev-parse --verify $(bisect_head)) ||
die "$(gettext "Bad rev input: $(bisect_head)")"
bisect_write "$state" "$rev"
-   check_expected_revs "$rev" ;;
+   git bisect--helper --check-expected-revs "$rev" ;;
2,"$TERM_BAD"|*,"$TERM_GOOD"|*,skip)
shift
hash_list=''
@@ -294,7 +278,7 @@ bisect_state() {
do
bisect_write "$state" "$rev"
done
-   check_expected_revs $hash_list ;;
+   git bisect--helper --check-expected-revs $hash_list ;;
*,"$TERM_BAD")
die "$(eval_gettext "'git bisect \$TERM_BAD' can take only one 
argument.")" ;;
*)
-- 
2.9.0

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

[PATCH v2 4/6] bisect--helper: `bisect_reset` shell function in C

[Pranit Bauva]

Reimplement `bisect_reset` shell function in C and add a `--bisect-reset`
subcommand to `git bisect--helper` to call it from git-bisect.sh .

Using `bisect_reset` subcommand is a temporary measure to port shell
functions to C so as to use the existing test suite. As more functions
are ported, this subcommand would be retired and will be called by some
other method.

Note: --bisect-clean-state subcommand has not been retired as there are
still a function namely `bisect_start()` which still uses this
subcommand.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 48 +++-
 git-bisect.sh| 28 ++--
 2 files changed, 49 insertions(+), 27 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 3e4a458..14043a8 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -4,6 +4,8 @@
 #include "bisect.h"
 #include "refs.h"
 #include "dir.h"
+#include "argv-array.h"
+#include "run-command.h"
 
 static GIT_PATH_FUNC(git_path_bisect_write_terms, "BISECT_TERMS")
 static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
@@ -13,11 +15,13 @@ static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
 static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
 static GIT_PATH_FUNC(git_path_head_name, "head-name")
 static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
+static GIT_PATH_FUNC(git_path_bisect_head, "BISECT_HEAD")
 
 static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --next-all [--no-checkout]"),
N_("git bisect--helper --write-terms  "),
N_("git bisect--helper --bisect-clean-state"),
+   N_("git bisect--helper --bisect-reset []"),
NULL
 };
 
@@ -123,12 +127,48 @@ static int bisect_clean_state(void)
return result;
 }
 
+static int bisect_reset(const char *commit)
+{
+   struct strbuf branch = STRBUF_INIT;
+
+   if (!commit) {
+   if (strbuf_read_file(, git_path_bisect_start(), 0) < 1) {
+   printf("We are not bisecting.\n");
+   return 0;
+   }
+   strbuf_rtrim();
+
+   } else {
+   struct object_id oid;
+   if (get_oid(commit, ))
+   return error(_("'%s' is not a valid commit"), commit);
+   strbuf_addf(, "%s", commit);
+   }
+
+   if (!file_exists(git_path_bisect_head())) {
+   struct argv_array argv = ARGV_ARRAY_INIT;
+   argv_array_pushl(, "checkout", branch.buf, "--", NULL);
+   if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
+   error(_("Could not check out original HEAD '%s'. Try"
+   "'git bisect reset '."), 
branch.buf);
+   strbuf_release();
+   argv_array_clear();
+   return -1;
+   }
+   argv_array_clear();
+   }
+
+   strbuf_release();
+   return bisect_clean_state();
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
NEXT_ALL = 1,
WRITE_TERMS,
-   BISECT_CLEAN_STATE
+   BISECT_CLEAN_STATE,
+   BISECT_RESET
} cmdmode = 0;
int no_checkout = 0;
struct option options[] = {
@@ -138,6 +178,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 N_("write the terms to .git/BISECT_TERMS"), 
WRITE_TERMS),
OPT_CMDMODE(0, "bisect-clean-state", ,
 N_("cleanup the bisection state"), BISECT_CLEAN_STATE),
+   OPT_CMDMODE(0, "bisect-reset", ,
+N_("reset the bisection state"), BISECT_RESET),
OPT_BOOL(0, "no-checkout", _checkout,
 N_("update BISECT_HEAD instead of checking out the 
current commit")),
OPT_END()
@@ -160,6 +202,10 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
if (argc != 0)
die(_("--bisect-clean-state requires no arguments"));
return bisect_clean_state();
+   case BISECT_RESET:
+   if (argc > 1)
+   die(_("--bisect-reset requires either zero or one 
arguments"));
+   return bisect_reset(argc ? argv[0] : NULL);
default:
die("BUG: unknown subcommand '%d'", cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index bbc57d2..18580b7 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -409,35 +409,11 @@ bisect_visualize() {
eval '"$@"' --bisect -- $(cat "$GIT_DIR/BISECT_NAMES")
 }
 
-bisect_reset() {
-   test -s "$GIT_DIR/BISECT_START" 

[PATCH v2 0/6] convert various shell functions in git-bisect to C

[Pranit Bauva]

A previous version is available here[1].

Changes wrt previous version:
 * Use STRING_LIST_INIT_NODUP to avoid leaks in bisect_clean_state()
 * Use test_path_is_missing in the patch 2/6
 * drop file_size()
 * move is_empty_file() method from builtin/am.c to wrapper.c
 * use static for methods
 * remove the variable status in bisect_reset() altogether and put the whole
   thing inside the if block.
 * one more method converted namely bisect_write().

An important thing to be discussed:
In shell script while reading/writing files, no error is reported but we can
actually report it in C. In this series I have tried to output error wherever
possible. What are your views?

[1]: http://thread.gmane.org/gmane.comp.version-control.git/296717

Pranit Bauva (6):
  bisect--helper: `bisect_clean_state` shell function in C
  t6030: explicitly test for bisection cleanup
  wrapper: move is_empty_file() from builtin/am.c
  bisect--helper: `bisect_reset` shell function in C
  bisect--helper: `is_expected_rev` & `check_expected_revs` shell
function in C
  bisect--helper: `bisect_write` shell function in C

 builtin/am.c|  16 
 builtin/bisect--helper.c| 194 +++-
 cache.h |   3 +
 git-bisect.sh   |  97 +++---
 t/t6030-bisect-porcelain.sh |  17 
 wrapper.c   |  13 +++
 6 files changed, 236 insertions(+), 104 deletions(-)

-- 
2.9.0

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

[PATCH v2 3/6] wrapper: move is_empty_file() from builtin/am.c

[Pranit Bauva]

is_empty_file() can help to refactor a lot of code. Also it is quite
helpful while converting shell scripts which use `test -s`. Since
is_empty_file() is now a "library" function, its inappropriate to die() so
instead error_errno() is used to convey the message to stderr while the
appropriate boolean value is returned.

Suggested-by: Torsten Bögershausen 
Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/am.c | 16 
 cache.h  |  3 +++
 wrapper.c| 13 +
 3 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 3dfe70b..84f21d0 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -30,22 +30,6 @@
 #include "mailinfo.h"
 
 /**
- * Returns 1 if the file is empty or does not exist, 0 otherwise.
- */
-static int is_empty_file(const char *filename)
-{
-   struct stat st;
-
-   if (stat(filename, ) < 0) {
-   if (errno == ENOENT)
-   return 1;
-   die_errno(_("could not stat %s"), filename);
-   }
-
-   return !st.st_size;
-}
-
-/**
  * Returns the length of the first line of msg.
  */
 static int linelen(const char *msg)
diff --git a/cache.h b/cache.h
index 6049f86..8eaad70 100644
--- a/cache.h
+++ b/cache.h
@@ -1870,4 +1870,7 @@ void sleep_millisec(int millisec);
  */
 void safe_create_dir(const char *dir, int share);
 
+/* Return 1 if the file is empty or does not exists, 0 otherwise. */
+extern int is_empty_file(const char *filename);
+
 #endif /* CACHE_H */
diff --git a/wrapper.c b/wrapper.c
index 5dc4e15..36a3eeb 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -696,3 +696,16 @@ void sleep_millisec(int millisec)
 {
poll(NULL, 0, millisec);
 }
+
+int is_empty_file(const char *filename)
+{
+   struct stat st;
+
+   if (stat(filename, ) < 0) {
+   if (errno == ENOENT)
+   return 1;
+   error_errno(_("could not stat %s"), filename);
+   }
+
+   return !st.st_size;
+}
-- 
2.9.0

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

[PATCH v2 2/6] t6030: explicitly test for bisection cleanup

[Pranit Bauva]

This is not an improvement in the test coverage but it helps in making
it explicit as to what exactly would be the error as other tests are
focussed on testing other things.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 

---
I faced this problem while converting `bisect_clean_state` and the tests
where showing breakages but it wasn't clear as to where exactly are they
breaking. This will patch  will help in that. Also I tested the test
coverage of the test suite before this patch and it covers this (I did
this by purposely changing names of files in git-bisect.sh and running
the test suite).

Signed-off-by: Pranit Bauva 
---
 t/t6030-bisect-porcelain.sh | 17 +
 1 file changed, 17 insertions(+)

diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index e74662b..a17f7a6 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -894,4 +894,21 @@ test_expect_success 'bisect start takes options and revs 
in any order' '
test_cmp expected actual
 '
 
+test_expect_success 'git bisect reset cleans bisection state properly' '
+   git bisect reset &&
+   git bisect start &&
+   git bisect good $HASH1 &&
+   git bisect bad $HASH4 &&
+   git bisect reset &&
+   test -z "$(git for-each-ref "refs/bisect/*")" &&
+   test_path_is_missing "$GIT_DIR/BISECT_EXPECTED_REV" &&
+   test_path_is_missing "$GIT_DIR/BISECT_ANCESTORS_OK" &&
+   test_path_is_missing "$GIT_DIR/BISECT_LOG" &&
+   test_path_is_missing "$GIT_DIR/BISECT_RUN" &&
+   test_path_is_missing "$GIT_DIR/BISECT_TERMS" &&
+   test_path_is_missing "$GIT_DIR/head-name" &&
+   test_path_is_missing "$GIT_DIR/BISECT_HEAD" &&
+   test_path_is_missing "$GIT_DIR/BISECT_START"
+'
+
 test_done
-- 
2.9.0

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

[PATCH v2 1/6] bisect--helper: `bisect_clean_state` shell function in C

[Pranit Bauva]

Reimplement `bisect_clean_state` shell function in C and add a
`bisect-clean-state` subcommand to `git bisect--helper` to call it from
git-bisect.sh .

Using `--bisect-clean-state` subcommand is a measure to port shell
function to C so as to use the existing test suite. As more functions
are ported, this subcommand will be retired and will be called by
bisect_reset() and bisect_start().

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 54 +++-
 git-bisect.sh| 26 +++
 2 files changed, 56 insertions(+), 24 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 91027b0..3e4a458 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -3,12 +3,21 @@
 #include "parse-options.h"
 #include "bisect.h"
 #include "refs.h"
+#include "dir.h"
 
 static GIT_PATH_FUNC(git_path_bisect_write_terms, "BISECT_TERMS")
+static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
+static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
+static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
+static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
+static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
+static GIT_PATH_FUNC(git_path_head_name, "head-name")
+static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
 
 static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --next-all [--no-checkout]"),
N_("git bisect--helper --write-terms  "),
+   N_("git bisect--helper --bisect-clean-state"),
NULL
 };
 
@@ -78,11 +87,48 @@ static int write_terms(const char *bad, const char *good)
return (res < 0) ? -1 : 0;
 }
 
+static int mark_for_removal(const char *refname, const struct object_id *oid,
+   int flag, void *cb_data)
+{
+   struct string_list *refs = cb_data;
+   char *ref = xstrfmt("refs/bisect/%s", refname);
+   string_list_append(refs, ref);
+   return 0;
+}
+
+static int bisect_clean_state(void)
+{
+   int result = 0;
+
+   /* There may be some refs packed during bisection */
+   struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
+   for_each_ref_in("refs/bisect/", mark_for_removal, (void *) 
_for_removal);
+   string_list_append(_for_removal, xstrdup("BISECT_HEAD"));
+   result = delete_refs(_for_removal);
+   string_list_clear(_for_removal, 0);
+   remove_path(git_path_bisect_expected_rev());
+   remove_path(git_path_bisect_ancestors_ok());
+   remove_path(git_path_bisect_log());
+   remove_path(git_path_bisect_names());
+   remove_path(git_path_bisect_run());
+   remove_path(git_path_bisect_write_terms());
+   /* Cleanup head-name if it got left by an old version of git-bisect */
+   remove_path(git_path_head_name());
+   /*
+* Cleanup BISECT_START last to support the --no-checkout option
+* introduced in the commit 4796e823a.
+*/
+   remove_path(git_path_bisect_start());
+
+   return result;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
NEXT_ALL = 1,
-   WRITE_TERMS
+   WRITE_TERMS,
+   BISECT_CLEAN_STATE
} cmdmode = 0;
int no_checkout = 0;
struct option options[] = {
@@ -90,6 +136,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 N_("perform 'git bisect next'"), NEXT_ALL),
OPT_CMDMODE(0, "write-terms", ,
 N_("write the terms to .git/BISECT_TERMS"), 
WRITE_TERMS),
+   OPT_CMDMODE(0, "bisect-clean-state", ,
+N_("cleanup the bisection state"), BISECT_CLEAN_STATE),
OPT_BOOL(0, "no-checkout", _checkout,
 N_("update BISECT_HEAD instead of checking out the 
current commit")),
OPT_END()
@@ -108,6 +156,10 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
if (argc != 2)
die(_("--write-terms requires two arguments"));
return write_terms(argv[0], argv[1]);
+   case BISECT_CLEAN_STATE:
+   if (argc != 0)
+   die(_("--bisect-clean-state requires no arguments"));
+   return bisect_clean_state();
default:
die("BUG: unknown subcommand '%d'", cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index cd39bd0..bbc57d2 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -187,7 +187,7 @@ bisect_start() {
#
# Get rid of any old bisect state.
#
-   bisect_clean_state || exit
+   git bisect--helper --bisect-clean-state || exit
 
#
  

[PATCH v2 6/6] bisect--helper: `bisect_write` shell function in C

[Pranit Bauva]

Reimplement the `bisect_write` shell function in C and add a
`bisect-write` subcommand to `git bisect--helper` to call it from
git-bisect.sh

Using `--bisect-write` subcommand is a temporary measure to port shell
function in C so as to use the existing test suite. As more functions
are ported, this subcommand will be retired and will be called by some
other methods.

Note: bisect_write() uses two variables namely TERM_GOOD and TERM_BAD
from the global shell script thus we need to pass it to the subcommand
using the arguments. After the whole conversion, we can remove the extra
arguments and make the method use the two variables from the global scope
within the C code.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 61 +++-
 git-bisect.sh| 25 
 2 files changed, 64 insertions(+), 22 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index f11c247..eebfcf0 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -22,6 +22,7 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --write-terms  "),
N_("git bisect--helper --bisect-clean-state"),
N_("git bisect--helper --bisect-reset []"),
+   N_("git bisect--helper --bisect-write
 []"),
NULL
 };
 
@@ -192,6 +193,55 @@ static int check_expected_revs(const char **revs, int 
rev_nr)
return 0;
 }
 
+static int bisect_write(const char *state, const char *rev,
+   const char *term_good, const char *term_bad,
+   int nolog)
+{
+   struct strbuf tag = STRBUF_INIT;
+   struct strbuf commit_name = STRBUF_INIT;
+   struct object_id oid;
+   struct commit *commit;
+   struct pretty_print_context pp = {0};
+   FILE *fp;
+
+   if (!strcmp(state, term_bad))
+   strbuf_addf(, "refs/bisect/%s", state);
+   else if(one_of(state, term_good, "skip", NULL))
+   strbuf_addf(, "refs/bisect/%s-%s", state, rev);
+   else
+   return error(_("Bad bisect_write argument: %s"), state);
+
+   if (get_oid(rev, )) {
+   strbuf_release();
+   return error(_("couldn't get the oid of the rev '%s'"), rev);
+   }
+
+   if (update_ref(NULL, tag.buf, oid.hash, NULL, 0,
+  UPDATE_REFS_MSG_ON_ERR)) {
+   strbuf_release();
+   return -1;
+   }
+
+   fp = fopen(git_path_bisect_log(), "a");
+   if (!fp) {
+   strbuf_release();
+   return error_errno(_("couldn't open the file '%s'"), 
git_path_bisect_log());
+   }
+
+   commit = lookup_commit_reference(oid.hash);
+   format_commit_message(commit, "%s", _name, );
+   fprintf(fp, "# %s: [%s] %s\n", state, sha1_to_hex(oid.hash),
+   commit_name.buf);
+
+   if (!nolog)
+   fprintf(fp, "git bisect %s %s\n", state, rev);
+
+   strbuf_release(_name);
+   strbuf_release();
+   fclose(fp);
+   return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
@@ -199,7 +249,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
WRITE_TERMS,
BISECT_CLEAN_STATE,
BISECT_RESET,
-   CHECK_EXPECTED_REVS
+   CHECK_EXPECTED_REVS,
+   BISECT_WRITE
} cmdmode = 0;
int no_checkout = 0;
struct option options[] = {
@@ -213,6 +264,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 N_("reset the bisection state"), BISECT_RESET),
OPT_CMDMODE(0, "check-expected-revs", ,
 N_("check for expected revs"), CHECK_EXPECTED_REVS),
+   OPT_CMDMODE(0, "bisect-write", ,
+N_("write out the bisection state in BISECT_LOG"), 
BISECT_WRITE),
OPT_BOOL(0, "no-checkout", _checkout,
 N_("update BISECT_HEAD instead of checking out the 
current commit")),
OPT_END()
@@ -225,6 +278,7 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
usage_with_options(git_bisect_helper_usage, options);
 
switch (cmdmode) {
+   int nolog;
case NEXT_ALL:
return bisect_next_all(prefix, no_checkout);
case WRITE_TERMS:
@@ -241,6 +295,11 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
return bisect_reset(argc ? argv[0] : NULL);
case CHECK_EXPECTED_REVS:
return check_expected_revs(argv, argc);
+   case BISECT_WRITE:
+   if (argc != 4 && argc != 5)
+   die(_("--bisect-write 

Re: Git clone 2.9.0

[Duy Nguyen]

On Wed, Jun 15, 2016 at 8:08 PM, Wojciech Rybiński
 wrote:
> Hi,
>
> I have installed version 2.9.0 and when I’m trying to clone repo with 
> specific tag I get error: unknown option `single-branch’ and next warning: 
> Remote branch a3.0.26 not found in upstream origin, using HEAD instead. My 
> command looks like that: git clone --single-branch -b a3.0.26 git@… and it’s 
> working with git version 2.7.4 on another machine. What should I do to clone 
> repo with this tag using version 2.9.0? I found info that this option is 
> enable in this version.

I can't reproduce. What does this command show to you?

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

Git clone 2.9.0

[Wojciech Rybiński]

Hi,

I have installed version 2.9.0 and when I’m trying to clone repo with specific 
tag I get error: unknown option `single-branch’ and next warning: Remote branch 
a3.0.26 not found in upstream origin, using HEAD instead. My command looks like 
that: git clone --single-branch -b a3.0.26 git@… and it’s working with git 
version 2.7.4 on another machine. What should I do to clone repo with this tag 
using version 2.9.0? I found info that this option is enable in this version.

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

Re: bug: compactionheuristic config var case issue

[Duy Nguyen]

On Wed, Jun 15, 2016 at 5:39 PM, Brian Lalor  wrote:
> I’m very happy to see the new compaction heuristic option; it’s the way I 
> always thought diffs should read!
>
> The config option in the documentation references “diff.compactionHeuristic”, 
> but diff.c does a case-sensitive comparison on “diff.compactionheuristic” 
> (note the case of the “h” in “heuristic”)

I think this misled you. All configuration variable names are
lower-cased before they reach that strcmp() call, the whole picture is
more like strcmp(tolower(var), "diff.compactionheuristic"), which I
believe is correct.

> and `git diff` does not honor the config.  Confusingly, `git config 
> diff.compactionheuristic` returns true when diff.compactionHeuristic is set 
> in ~/.gitconfig.  When diff.compactionheuristic is set to true in 
> ~/.gitconfig, the desired behavior is achieved.
>
> Thank you all for Git: it’s hard to remember the terrible world we lived in 
> before it existed. :-)
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bug report: stdout vs stderr

[Victor Porton]

Why half of Git output goes to stdout and half to stderr? I suspect
this is a bug.

Below I call `git pushbug` alias defined it the below presented config
file.

$ cat .git/config 
[core]
repositoryformatversion = 0
filemode = true
bare = false
logallrefupdates = true
[remote "origin"]
url = g...@bitbucket.org:portonv/algebraic-general-topology.git
fetch = +refs/heads/*:refs/remotes/origin/*
pushurl = g...@github.com:vporton/algebraic-general-topology.git
pushurl = g...@bitbucket.org:portonv/algebraic-general-
topology.git
[gui]
wmstate = normal
geometry = 1680x957+0+27 189 177
[alias]
pushbug = !git push && git checkout prerelease && git merge
master && git push && git checkout devel && git merge prerelease &&
git push && git checkout master
[branch "master"]
remote = origin
merge = refs/heads/master
[branch "prerelease"]
remote = origin
merge = refs/heads/prerelease
[branch "devel"]
remote = origin
merge = refs/heads/devel

$ git pushbug 1>$HOME/t/1.txt 2>$HOME/t/2.txt
$ cat ~/t/1.txt 
Your branch is up-to-date with 'origin/prerelease'.
Updating ac492a4..c55d1b5
Fast-forward
 chap-sides.tex | 15 +--
 1 file changed, 13 insertions(+), 2 deletions(-)
Your branch is up-to-date with 'origin/devel'.
Updating ac492a4..c55d1b5
Fast-forward
 chap-sides.tex | 15 +--
 1 file changed, 13 insertions(+), 2 deletions(-)
Your branch is up-to-date with 'origin/master'.
$ cat ~/t/2.txt 
To g...@github.com:vporton/algebraic-general-topology.git
   ac492a4..c55d1b5  master -> master
To g...@bitbucket.org:portonv/algebraic-general-topology.git
   ac492a4..c55d1b5  master -> master
Switched to branch 'prerelease'
To g...@github.com:vporton/algebraic-general-topology.git
   ac492a4..c55d1b5  prerelease -> prerelease
remote: 
remote: Create pull request for prerelease:
remote:   https://bitbucket.org/portonv/algebraic-general-topology/pull
-requests/new?source=prerelease=1
remote: 
To g...@bitbucket.org:portonv/algebraic-general-topology.git
   ac492a4..c55d1b5  prerelease -> prerelease
Switched to branch 'devel'
To g...@github.com:vporton/algebraic-general-topology.git
   ac492a4..c55d1b5  devel -> devel
remote: 
remote: Create pull request for devel:
remote:   https://bitbucket.org/portonv/algebraic-general-topology/pull
-requests/new?source=devel=1
remote: 
To g...@bitbucket.org:portonv/algebraic-general-topology.git
   ac492a4..c55d1b5  devel -> devel
Switched to branch 'master'
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[ANNOUNCE] Git Rev News edition 16

[Christian Couder]

Hi everyone,

I'm happy announce that the 16th edition of Git Rev News is now published:

http://git.github.io/rev_news/2016/06/15/edition-16/

Thanks a lot to all the contributors and helpers, especially Duy and
the Ensimag students!

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

[PATCH] verify-tag: allow to verify signed blob objects

[Michael J Gruber]

Currently, there is no easy way to verify push certificates. They have
the same structure as signed tags: "attached detached signatures", that
is: the concatenation of the signed material and its detached signature.

Introduce a `--blob` option to verify-tag so that it allows to verify
tags and blobs.

Signed-off-by: Michael J Gruber 
---
The first outcome of my long announced project to describe our signature
formats in Documentation/technical (progress underway)

In fact, that whole area is in need of refactoring: gpg related bits are
all over the place, including tag.c. The proposed patch neither improves
nor worsens the situation in that respect. But, since we make it
unnecessarily hard to verify signatures (git cat-file | gpg --verify fails)
it's only fair to provide a tool for pre-receive hook writers.

 Documentation/git-verify-tag.txt | 4 
 builtin/verify-tag.c | 1 +
 gpg-interface.h  | 1 +
 t/t5534-push-signed.sh   | 3 ++-
 tag.c| 2 +-
 5 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-verify-tag.txt b/Documentation/git-verify-tag.txt
index d590edc..2e5cf4d 100644
--- a/Documentation/git-verify-tag.txt
+++ b/Documentation/git-verify-tag.txt
@@ -20,6 +20,10 @@ OPTIONS
Print the raw gpg status output to standard error instead of the normal
human-readable output.
 
+--blob::
+   Allow to verify signed blob objects (in addition to tag objects), such 
as the
+   objects containing a push certificate.
+
 -v::
 --verbose::
Print the contents of the tag object before validating it.
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 99f8148..19d26b0 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -33,6 +33,7 @@ int cmd_verify_tag(int argc, const char **argv, const char 
*prefix)
const struct option verify_tag_options[] = {
OPT__VERBOSE(, N_("print tag contents")),
OPT_BIT(0, "raw", , N_("print raw gpg status output"), 
GPG_VERIFY_RAW),
+   OPT_BIT(0, "blob", , N_("allow to verify blob objects"), 
GPG_VERIFY_BLOB),
OPT_END()
};
 
diff --git a/gpg-interface.h b/gpg-interface.h
index ea68885..a3cbfc3 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -3,6 +3,7 @@
 
 #define GPG_VERIFY_VERBOSE 1
 #define GPG_VERIFY_RAW 2
+#define GPG_VERIFY_BLOB4
 
 struct signature_check {
char *payload;
diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
index ecb8d44..de4d38b 100755
--- a/t/t5534-push-signed.sh
+++ b/t/t5534-push-signed.sh
@@ -94,7 +94,8 @@ test_expect_success GPG 'signed push sends push certificate' '
# record the push certificate
if test -n "${GIT_PUSH_CERT-}"
then
-   git cat-file blob $GIT_PUSH_CERT >../push-cert
+   git cat-file blob $GIT_PUSH_CERT >../push-cert &&
+   git verify-tag --blob $GIT_PUSH_CERT
fi &&
 
cat >../push-cert-status 

Re: [PATCH] hooks--pre-commit.sample: check for chars, that are not allowed for a windows file name

[Thomas Braun]

Am 15.06.2016 um 10:02 schrieb dexteritas:
> After the ASCII-check, test the windows compatibility of file names.
> Can be disabled by:
> git config hooks.allownonwindowschars true
> ---
>  templates/hooks--pre-commit.sample | 22 ++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/templates/hooks--pre-commit.sample 
> b/templates/hooks--pre-commit.sample
> index 68d62d5..120daf1 100755
> --- a/templates/hooks--pre-commit.sample
> +++ b/templates/hooks--pre-commit.sample
> @@ -17,6 +17,7 @@ fi
>  
>  # If you want to allow non-ASCII filenames set this variable to true.
>  allownonascii=$(git config --bool hooks.allownonascii)
> +allownonwindowschars=$(git config --bool hooks.allownonwindowschars)
>  
>  # Redirect output to stderr.
>  exec 1>&2
> @@ -43,6 +44,27 @@ If you know what you are doing you can disable this check 
> using:
>git config hooks.allownonascii true
>  EOF
>   exit 1
> +elif [ "$allownonwindowschars" != "true" ] &&
> + # If you work with linux and windows, there is a problem, if you use
> + # chars like \ / : * ? " < > |
> + # Check if there are used only windows compatible chars
> + test $(git diff --cached --name-only --diff-filter=A -z $against |
> +   LC_ALL=C tr -d '[0-9A-Za-z\[\]\{\}_ -)+-.]\0' | wc -c) != 0
> +then
> + cat <<\EOF
> +Error: Attempt to add a chars that are not allowed for a windows file name.
> +
> +This can cause problems if you want to work with people on other platforms.
> +
> +To be portable it is advisable to rename the file.
> +
> +Check your filenames for: \ / : * ? " < > |
> +
> +If you know what you are doing you can disable this check using:
> +
> +  git config hooks.allownonwindowschars true
> +EOF
> + exit 2
>  fi
>  
>  # If there are whitespace errors, print the offending file names and fail.

There are some cases of illegal file names missing. E.g. reserved names,
trailing period and space. My trial with a precommit hook for avoiding
illegal filenames on windows can be found at [1]. Feel free to loot my
version for a reroll.

[1]:
https://github.com/t-b/git-pre-commit-hook-windows-filenames/blob/master/pre-commit

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

bug: compactionheuristic config var case issue

[Brian Lalor]

I’m very happy to see the new compaction heuristic option; it’s the way I 
always thought diffs should read!  

The config option in the documentation references “diff.compactionHeuristic”, 
but diff.c does a case-sensitive comparison on “diff.compactionheuristic” (note 
the case of the “h” in “heuristic”) and `git diff` does not honor the config.  
Confusingly, `git config diff.compactionheuristic` returns true when 
diff.compactionHeuristic is set in ~/.gitconfig.  When diff.compactionheuristic 
is set to true in ~/.gitconfig, the desired behavior is achieved.

Thank you all for Git: it’s hard to remember the terrible world we lived in 
before it existed. :-)--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: I lost my commit signature

[ZhenTian]

Thank you all very much!
-Schrödinger


On Wed, Jun 15, 2016 at 3:07 PM, Michael J Gruber
 wrote:
> Jeff King venit, vidit, dixit 15.06.2016 06:34:
>> On Wed, Jun 15, 2016 at 12:27:15PM +0800, ZhenTian wrote:
>>
>>> I got two more lines from gpg -v during commit with -S:
>>> ```
>>> gpg: writing to stdout
>>> gpg: RSA/SHA1 signature from: "2EF2AD6E Tian Zhen "
>>> ```
>>>
>>> after I commit, I push it to remote, but someone had pushed before to
>>> master branch, so I pull on master branch(`git pull --rebase`), then I
>>> check my commit via `git log --show-signature`, there is no signature
>>> in it, so I commit it with --ament and -S again, the signature is come
>>> back.
>>>
>>> I haven't check signature before push, because I have checked four
>>> commits before, every commit is fine.
>>>
>>> I don't know whether the `git pull` influenced signature or not.
>>
>> Ah, so the problem is probably that you had a signature _initially_, but
>> that it did not survive the rebase. Which makes sense, as rebase would
>> need to re-sign.  It does not by default, but you can tell it to do so
>> with "-S". Or you can set `commit.gpgsign`, which should sign in both
>> cases.
>
> While it's clear that a rebase invalidates the signature, we could try
> to be more helpful here, especially given the fact that (with our model)
> you can't sign a commit afterwards any more.
>
> commit.gpgsign signs everything, but there should be a mode for
> re-signing signed commits, or at least a warning that rebase dropped a
> signature so that you can --amend -S the last commit.
>
> Michael
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What's cooking in git.git (Jun 2016, #04; Tue, 14)

[Duy Nguyen]

On Wed, Jun 15, 2016 at 5:08 AM, Junio C Hamano  wrote:
> * nd/i-t-a-commitable (2016-06-06) 3 commits
>  - commit: don't count i-t-a entries when checking if the new commit is empty
>  - Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"
>  - diff.h: extend "flags" field to 64 bits because we're out of bits
>
>  "rm .git/index && git add -N * && git commit" allows you to create
>  an empty commit without --allow-empty; attempt to forbid it.
>
>  Breaks many tests by completely butchering "git commit", it seems.

Not surprising. I did run some basic tests, but not the test suite. It
was more an excuse to bring up the topic again. Please drop it. I will
probably resend (with more or less the same idea, since you haven't
given a loud and clear "NO").

> * nd/worktree-cleanup-post-head-protection (2016-05-24) 6 commits
>  - worktree: simplify prefixing paths
>  - worktree: avoid 0{40}, too many zeroes, hard to read
>  - worktree.c: use is_dot_or_dotdot()
>  - git-worktree.txt: keep subcommand listing in alphabetical order
>  - worktree.c: rewrite mark_current_worktree() to avoid strbuf
>  - completion: support git-worktree
>  (this branch is used by nd/worktree-lock.)
>
>  Further preparatory clean-up for "worktree" feature.
>
>  Expecting a reroll.
>  ($gmane/294136, etc.)

Hmm.. I think what's in 'pu' (which is v2, $gmane/295260) is ok now.
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [ANNOUNCE] Sharness v1.0.0

[Mathias Lafeldt]

>> Is there any word out there from Mathias on making you the designated
>> new maintainer? (I cannot tell if this is a genuine maintainer change, or
>> a [hostile] fork by reading this email, and I don't know much of the context,

Yes, it's 100% genuine. I handed over maintenance to Christian Couder.

Here's a signed commit to prove the transfer:
https://github.com/chriscool/sharness/pull/55

(Sorry for double posting, the mailing list didn't like my first email.)

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

[PATCH] hooks--pre-commit.sample: check for chars, that are not allowed for a windows file name

[dexteritas]

After the ASCII-check, test the windows compatibility of file names.
Can be disabled by:
git config hooks.allownonwindowschars true
---
 templates/hooks--pre-commit.sample | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/templates/hooks--pre-commit.sample 
b/templates/hooks--pre-commit.sample
index 68d62d5..120daf1 100755
--- a/templates/hooks--pre-commit.sample
+++ b/templates/hooks--pre-commit.sample
@@ -17,6 +17,7 @@ fi
 
 # If you want to allow non-ASCII filenames set this variable to true.
 allownonascii=$(git config --bool hooks.allownonascii)
+allownonwindowschars=$(git config --bool hooks.allownonwindowschars)
 
 # Redirect output to stderr.
 exec 1>&2
@@ -43,6 +44,27 @@ If you know what you are doing you can disable this check 
using:
   git config hooks.allownonascii true
 EOF
exit 1
+elif [ "$allownonwindowschars" != "true" ] &&
+   # If you work with linux and windows, there is a problem, if you use
+   # chars like \ / : * ? " < > |
+   # Check if there are used only windows compatible chars
+   test $(git diff --cached --name-only --diff-filter=A -z $against |
+ LC_ALL=C tr -d '[0-9A-Za-z\[\]\{\}_ -)+-.]\0' | wc -c) != 0
+then
+   cat <<\EOF
+Error: Attempt to add a chars that are not allowed for a windows file name.
+
+This can cause problems if you want to work with people on other platforms.
+
+To be portable it is advisable to rename the file.
+
+Check your filenames for: \ / : * ? " < > |
+
+If you know what you are doing you can disable this check using:
+
+  git config hooks.allownonwindowschars true
+EOF
+   exit 2
 fi
 
 # If there are whitespace errors, print the offending file names and fail.

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

Re: What's cooking in git.git (Jun 2016, #04; Tue, 14)

[Elia Pinto]

2016-06-15 0:08 GMT+02:00 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.
>
> Git 2.9 has been tagged.  Let's wait for a few days to clean up
> possible fallout and then start a new cycle by rewinding the tip of
> 'next'.  I expect I'd eject a few premature topics out of 'next'
> while doing so.
>
> 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
>
> --
> [Graduated to "master"]
>
> * jc/t2300-setup (2016-06-01) 1 commit
>   (merged to 'next' on 2016-06-06 at 20f7f83)
>  + t2300: run git-sh-setup in an environment that better mimics the real life
>  (this branch is used by va/i18n-even-more.)
>
>  A test fix.
>
>
> * jk/diff-compact-heuristic (2016-06-10) 1 commit
>  - diff: disable compaction heuristic for now
>
>  It turns out that the earlier effort to update the heuristics may
>  want to use a bit more time to mature.  Turn it off by default.
>
>
> * jk/shell-portability (2016-06-01) 2 commits
>   (merged to 'next' on 2016-06-06 at 5de784e)
>  + t5500 & t7403: lose bash-ism "local"
>  + test-lib: add in-shell "env" replacement
>
>  test fixes.
>
> --
> [New Topics]
>
> * lv/status-say-working-tree-not-directory (2016-06-09) 1 commit
>  - Use "working tree" instead of "working directory" for git status
>
>  "git status" used to say "working directory" when it meant "working
>  tree".
>
>  Will merge to 'next'.
>
>
> * jk/parseopt-string-list (2016-06-13) 3 commits
>  - blame,shortlog: don't make local option variables static
>  - interpret-trailers: don't duplicate option strings
>  - parse_opt_string_list: stop allocating new strings
>  (this branch is used by jk/string-list-static-init.)
>
>  The command line argument parsing that uses OPT_STRING_LIST() often
>  made a copy of the argv[] element, which was unnecessary.
>
>  Will merge to 'next'.
>
>
> * jk/repack-keep-unreachable (2016-06-14) 3 commits
>  - repack: extend --keep-unreachable to loose objects
>  - repack: add --keep-unreachable option
>  - repack: document --unpack-unreachable option
>
>  "git repack" learned the "--keep-unreachable" option, which sends
>  loose unreachable objects to a pack instead of leaving them loose.
>  This helps heuristics based on the number of loose objects
>  (e.g. "gc --auto").
>
>  Will merge to 'next'.
>
>
> * lf/recv-sideband-cleanup (2016-06-13) 1 commit
>  - sideband.c: refactor recv_sideband()
>
>  Code simplification.  It however loses the atomicity of the output
>  9ac13ec9 (atomic write for sideband remote messages, 2006-10-11)
>  tried to add to an once-much-simpler codebase.
>
>  Expecting a reroll.
>
>
> * nd/test-lib-httpd-show-error-log-in-verbose (2016-06-13) 1 commit
>  - lib-httpd.sh: print error.log on error
>
>  Debugging aid.
>
>  Will merge to 'next'.
>
>
> * pc/occurred (2016-06-10) 2 commits
>  - config.c: fix misspelt "occurred" in an error message
>  - refs.h: fix misspelt "occurred" in a comment
>
>  Will merge to 'next'.
>
>
> * sb/submodule-clone-retry (2016-06-13) 2 commits
>  - submodule update: continue when a clone fails
>  - submodule--helper: initial clone learns retry logic
>  (this branch uses sb/submodule-recommend-shallowness.)
>
>  "git submodule update" that drives many "git clone" could
>  eventually hit flaky servers/network conditions on one of the
>  submodules; the command learned to retry the attempt.
>
>
> * jc/blame-reverse (2016-06-14) 2 commits
>  - blame: dwim "blame --reverse OLD" as "blame --reverse OLD.."
>  - blame: improve diagnosis for "--reverse NEW"
>
>
> * jc/deref-tag (2016-06-14) 1 commit
>  - blame, line-log: do not loop around deref_tag()
>
>  Code clean-up.
>
>  Will merge to 'next'.
>
>
> * jk/fetch-prune-doc (2016-06-14) 1 commit
>  - fetch: document that pruning happens before fetching
>
>  Will merge to 'next'.
>
>
> * km/fetch-do-not-free-remote-name (2016-06-14) 1 commit
>  - builtin/fetch.c: don't free remote->name after fetch
>
>  Will merge to 'next'.
>
>
> * nb/gnome-keyring-build (2016-06-14) 1 commit
>  - gnome-keyring: Don't hard-code pkg-config executable
>
>  Build improvements for gnome-keyring (in contrib/)
>
>  Will merge to 'next'.
>
>
> * pb/strbuf-read-file-doc (2016-06-14) 1 commit
>  - strbuf: describe the return value of strbuf_read_file
>
>  Will merge to 'next'.
>
> --
> [Stalled]
>
> * sb/bisect (2016-04-15) 22 commits
>  - SQUASH???
>  - bisect: get back halfway shortcut
>  - bisect: compute best bisection in compute_relevant_weights()
>  - bisect: use a bottom-up traversal to find relevant weights
>  

Re: [PATCHv3] gpg-interface: check gpg signature creation status

[Michael J Gruber]

Jeff King venit, vidit, dixit 15.06.2016 02:56:
> On Tue, Jun 14, 2016 at 04:47:35PM -0700, Junio C Hamano wrote:
> 
>> Jeff King  writes:
>>
>>> I'm still undecided on whether it is a better approach than making
>>> sure the stdout we got looks sane. In particular I'd worry that it
>>> would make things harder for somebody trying to plug in something
>>> gpg-like (e.g., if you wanted to do something exotic like call a
>>> program which fetched the signature from a remote device or
>>> something).  But it's probably not _that_ hard for such a script
>>> to emulate --status-fd.
>>
>> I share the same thinking, but at the same time, it already is a
>> requirement to give --status-fd output that is close enough on the
>> signature verification side, isn't it?
> 
> Yeah, though I could see somebody wanting to sit amidst the signing side
> but not verification (e.g., if your keys are elsewhere from the machine
> running git). Of course such a case could probably ferry --status-fd
> from the other side anyway.
> 
> I admit I don't know of such a case in practice, though, and
> implementing a rudimentary --status-fd to say "SIG OK" or whatever on
> the signing side is not _that_ big a deal. So if we think this approach
> is a more robust solution in the normal case, let's not hold it up over
> what-ifs.
> 
> -Peff

As for the flexibility:
We do code specifically for gpg, which happens to work for gpg2 also.
The patch doesn't add any gpg ui requirements that we don't require
elsewhere already.
More flexibility requires a completely pluggable approach - gpgsm
already fails to meet the gpg command line ui.

In any case, "status-fd" is *the* way to interface with gpg reliably
just like plumbing commands are *the* way to interface with git reliably.

As for the read locking:
I'm sorry I have no idea about that area at all. I thought that I'm
doing the same that we do for verify, but apparently not. My
strbuf_read-fu is not that strong either (read: copy). I trust
your assessment completely, though.

As for the original problem:
That had a different cause, as we know now (rebase dropping signatures
without hint). I still think we should check gpg status codes properly.

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

Re: I lost my commit signature

[Michael J Gruber]

Jeff King venit, vidit, dixit 15.06.2016 06:34:
> On Wed, Jun 15, 2016 at 12:27:15PM +0800, ZhenTian wrote:
> 
>> I got two more lines from gpg -v during commit with -S:
>> ```
>> gpg: writing to stdout
>> gpg: RSA/SHA1 signature from: "2EF2AD6E Tian Zhen "
>> ```
>>
>> after I commit, I push it to remote, but someone had pushed before to
>> master branch, so I pull on master branch(`git pull --rebase`), then I
>> check my commit via `git log --show-signature`, there is no signature
>> in it, so I commit it with --ament and -S again, the signature is come
>> back.
>>
>> I haven't check signature before push, because I have checked four
>> commits before, every commit is fine.
>>
>> I don't know whether the `git pull` influenced signature or not.
> 
> Ah, so the problem is probably that you had a signature _initially_, but
> that it did not survive the rebase. Which makes sense, as rebase would
> need to re-sign.  It does not by default, but you can tell it to do so
> with "-S". Or you can set `commit.gpgsign`, which should sign in both
> cases.

While it's clear that a rebase invalidates the signature, we could try
to be more helpful here, especially given the fact that (with our model)
you can't sign a commit afterwards any more.

commit.gpgsign signs everything, but there should be a mode for
re-signing signed commits, or at least a warning that rebase dropped a
signature so that you can --amend -S the last commit.

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

Re: compactionHeuristic=true is not used by interactive staging

[Alex Prengère]

I see, it makes sense ;-) Indeed it would seem logical to have all
commands showing diffs (diff, add -p, log -p, reset -p, etc..) respect
the diff options.

Thanks for your quick answer!

2016-06-14 23:45 GMT+02:00 Junio C Hamano :
> Jeff King  writes:
>
>> Nobody noticed so far because originally the compaction heuristic was on
>> by default, and so just worked everywhere. But we backed off on that at
>> the last minute after finding a few cases where the diff looks worse.
>
> Yup, and that is why this is called "experimental" in the release
> notes ;-)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html