Re: [PATCH v3 1/1] check-non-portable-shell.pl: Quoted `wc -l` is not portable

[Torsten Bögershausen]

On Fri, Dec 22, 2017 at 01:07:59PM -0800, Junio C Hamano wrote:
> tbo...@web.de writes:
> 
> >
> > Reviewed-by: Johannes Schindelin 
> > Signed-off-by: Torsten Bögershausen 
> 

> I'll flip these and add a helped-by to credit Eric.
...
> Don't try to apply this patch to your tree yourself ;-)

Thanks so much for cleaning up my mess.

Bring together merge and rebase

[Carl Baldwin]

The big contention among git users is whether to rebase or to merge
changes [2][3] while iterating. I used to firmly believe that merging
was the way to go and rebase was harmful. More recently, I have worked
in some environments where I saw rebase used very effectively while
iterating on changes and I relaxed my stance a lot. Now, I'm on the
fence. I appreciate the strengths and weaknesses of both approaches. I
waffle between the two depending on the situation, the tools being
used, and I guess, to some extent, my mood.

I think what git needs is something brand new that brings the two
together and has all of the advantages of both approaches. Let me
explain what I've got in mind...

I've been calling this proposal `git replay` or `git replace` but I'd
like to hear other suggestions for what to name it. It works like
rebase except with one very important difference. Instead of orphaning
the original commit, it keeps a pointer to it in the commit just like
a `parent` entry but calls it `replaces` instead to distinguish it
from regular history. In the resulting commit history, following
`parent` pointers shows exactly the same history as if the commit had
been rebased. Meanwhile, the history of iterating on the change itself
is available by following `replaces` pointers. The new commit replaces
the old one but keeps it around to record how the change evolved.

The git history now has two dimensions. The first shows a cleaned up
history where fix ups and code review feedback have been rolled into
the original changes and changes can possibly be ordered in a nice
linear progression that is much easier to understand. The second
drills into the history of a change. There is no loss and you don't
change history in a way that will cause problems for others who have
the older commits.

Replay handles collaboration between multiple authors on a single
change. This is difficult and prone to accidental loss when using
rebase and it results in a complex history when done with merge. With
replay, collaborators could merge while collaborating on a single
change and a record of each one's contributions can be preserved.
Attempting this level of collaboration caused me many headaches when I
worked with the gerrit workflow (which in many ways, I like a lot).

I blogged about this proposal earlier this year when I first thought
of it [1]. I got busy and didn't think about it for a while. Now with
a little time off of work, I've come back to revisit it. The blog
entry has a few examples showing how it works and how the history will
look in a few examples. Take a look.

Various git commands will have to learn how to handle this kind of
history. For example, things like fetch, push, gc, and others that
move history around and clean out orphaned history should treat
anything reachable through `replaces` pointers as precious. Log and
related history commands may need new switches to traverse the history
differently in different situations. Bisect is a interesting one. I
tend to think that bisect should prefer the regular commit history but
have the ability to drill into the change history if necessary.

In my opinion, this proposal would bring together rebase and merge in
a powerful way and could end the contention. Thanks for your
consideration.

Carl Baldwin

[1] http://blog.episodicgenius.com/post/merge-or-rebase--neither/
[2] https://git-scm.com/book/en/v2/Git-Branching-Rebasing
[3] http://changelog.complete.org/archives/586-rebase-considered-harmful

Re: Usability outrage as far as I am concerned

[Anatolii Borodin]

Hi Jacob,

On Sat, Dec 23, 2017 at 12:05 AM, Jacob Keller  wrote:
> If you wish to update it later, you can mount hte usb stick, and then
> just run git pull from inside the new subfolder. Note that you do
> *not* run "git pull home_subfolder", as git pull expects the name of a
> remote, which in this case is just origin (since the default remote
> name you clone from is origin)

>From git-fetch(8):

   
   The "remote" repository that is the source of a fetch or
pull operation.
   This parameter can be either a URL (see the section GIT URLS below)
   or the name of a remote (see the section REMOTES below).

You can run git fetch / git pull with a URL or a local path to a
repository, not only origin etc.


-- 
Mit freundlichen Grüßen,
Anatolii Borodin

Re: [PATCH 2/3] move index_has_changes() from builtin/am.c to merge.c for reuse

[Elijah Newren]

On Fri, Dec 22, 2017 at 12:46 PM, Junio C Hamano  wrote:
> Elijah Newren  writes:
>
>> On Thu, Dec 21, 2017 at 11:19 AM, Elijah Newren  wrote:
>>> index_has_changes() is a function we want to reuse outside of just am,
>>> making it also available for merge-recursive and merge-ort.
>>>
>>> Signed-off-by: Elijah Newren 
>>> ---
>>
>> Note: These patches built on master, and merge cleanly with next and
>> pu.  However, this patch has a minor conflict with maint.  If you'd
>> prefer a version that applies on top of maint, let me know and I'll
>> resubmit.
>
> I think I managed to create two topics, one that is with these three
> patches (2/3 backported) on top of maint and the other one merges
> the former on top of master.  Please see if you found a mismerge
> when I push the results out.

I'm about to head out on a multi-day trip, so I might not get to this
until the middle of next week.  I'll try to take a look as soon as I
can, though.

Re: [PATCH] Fix urlencode format string on signed char.

[Johannes Schindelin]

Hi Julien,

On Sat, 23 Dec 2017, Julien Dusser wrote:

> Thank you for review. I didn't find any other error.
> Code in http.c:quote_ref_url() is almost the same but ch is a signed int, so
> there's no issue.

But that ch comes from a signed char *, so it actually *is* an issue: if
you cast a signed char of value -1 to an int, it will still be -1. So
we'll also need:

-- snipsnap --
diff --git a/http.c b/http.c
index 117ddae..ed8221f 100644
--- a/http.c
+++ b/http.c
@@ -1347,7 +1347,7 @@ void finish_all_active_slots(void)
 }
 
 /* Helpers for modifying and creating URLs */
-static inline int needs_quote(int ch)
+static inline int needs_quote(unsigned char ch)
 {
if (((ch >= 'A') && (ch <= 'Z'))
|| ((ch >= 'a') && (ch <= 'z'))
@@ -1363,11 +1363,11 @@ static char *quote_ref_url(const char *base, const char 
*ref)
 {
struct strbuf buf = STRBUF_INIT;
const char *cp;
-   int ch;
+   unsigned char ch;
 
end_url_with_slash(, base);
 
-   for (cp = ref; (ch = *cp) != 0; cp++)
+   for (cp = ref; (ch = (unsigned char)*cp); cp++)
if (needs_quote(ch))
strbuf_addf(, "%%%02x", ch);
else

Re: [PATCH] oidmap: ensure map is initialized

[Johannes Schindelin]

Hi Brandon,

On Fri, 22 Dec 2017, Brandon Williams wrote:

> Ensure that an oidmap is initialized before attempting to add, remove,
> or retrieve an entry by simply performing the initialization step
> before accessing the underlying hashmap.
> 
> Signed-off-by: Brandon Williams 
> ---
>  oidmap.c | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/oidmap.c b/oidmap.c
> index 6db4fffcd..d9fb19ba6 100644
> --- a/oidmap.c
> +++ b/oidmap.c
> @@ -33,12 +33,19 @@ void oidmap_free(struct oidmap *map, int free_entries)
>  
>  void *oidmap_get(const struct oidmap *map, const struct object_id *key)
>  {
> + if (!map->map.cmpfn)
> + return NULL;
> +

This one is unnecessary: if we always _init() before we add, then
hashmap_get_from_hash() will not have anything to compare, and therefore
not call cmpfn.

You could argue that it is only a teeny-tiny runtime cost, so it'd be
better to be safe than to be sorry.

However, it is a death by a thousand cuts. My colleagues and I try to
shave off a few percent here and a few percent there, in the hope to get
some performance improvements worth writing home about. And then patches
like this one come in that simply add runtime without much in the way of
performance considerations.

And that makes me quite a bit sad.

>   return hashmap_get_from_hash(>map, hash(key), key);
>  }
>  
>  void *oidmap_remove(struct oidmap *map, const struct object_id *key)
>  {
>   struct hashmap_entry entry;
> +
> + if (!map->map.cmpfn)
> + oidmap_init(map, 0);
> +
>   hashmap_entry_init(, hash(key));
>   return hashmap_remove(>map, , key);
>  }
> @@ -46,6 +53,10 @@ void *oidmap_remove(struct oidmap *map, const struct 
> object_id *key)
>  void *oidmap_put(struct oidmap *map, void *entry)
>  {
>   struct oidmap_entry *to_put = entry;
> +
> + if (!map->map.cmpfn)
> + oidmap_init(map, 0);
> +
>   hashmap_entry_init(_put->internal_entry, hash(_put->oid));
>   return hashmap_put(>map, to_put);
>  }

"But it does not add a lot, it's only a couple of microseconds"

Sure. But we could (and do) simply initialize the hashmaps once, and avoid
having to spend unnecessary cycles for every single access.

I *much* prefer my original patch that essentially does not change *any*
code path. Everything stays the same, except that there is now a strong
hint explaining why you need to call oidmap_init() manually instead of
using the OIDMAP_INIT macro and then wonder why your code crashes.

Ciao,
Dscho

Re: [PATCH] oidmap.h: strongly discourage using OIDMAP_INIT directly

[Johannes Schindelin]

Hi Junio,

On Fri, 22 Dec 2017, Junio C Hamano wrote:

> Brandon Williams  writes:
> 
> >> -#define OIDMAP_INIT { { NULL } }
> >> +/*
> >> + * This macro initializes the data structure only incompletely, just 
> >> enough
> >> + * to call oidmap_get() on an empty map. Use oidmap_init() instead.
> >> + */
> >> +#define OIDMAP_INIT_INCOMPLETELY { { NULL } }
> >
> > This is one way of approaching the problem.  Couldn't we also take the
> > approach like we have with oidset and ensure that when oidmap_get() or
> > _put() are called that if the oidmap isn't initialized, we simply do
> > that then?
> 
> Hmph.  Can you show how the alternative code would look like?

No, because I refuse to perform pointless work, in particular when I am
already pretty booked with work.

But you know how it would look like, right? The cmpfn() function would be
exported via oidmap.h, and a HASHMAP_INIT(cmpfn) would be introduced in
hashmap.h that would initialize everything zeroed out except for the
cmpfn.

But then you would review it and ask if there would be any use in adding
cmp_cb_data to the signature of the HASHMAP_INIT() macro, and I would have
to implement that, too.

And then nobody would use it, and the macro would very likely get
stale/incorrect. And then I would offer another patch reverting that
change (because there is no user) and replace it with this here patch.

As I said: pointless,
Dscho

[PATCH 5/5] sequencer: do not invent whitespace when transforming OIDs

[Johannes Schindelin]

For commands that do not have an argument, there is no need to append a
trailing space at the end of the line.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 5632415ea2d..970842e3fcc 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2584,7 +2584,10 @@ int transform_todos(unsigned flags)
strbuf_addf(, " %s", oid);
}
/* add all the rest */
-   strbuf_addf(, " %.*s\n", item->arg_len, item->arg);
+   if (!item->arg_len)
+   strbuf_addch(, '\n');
+   else
+   strbuf_addf(, " %.*s\n", item->arg_len, item->arg);
}
 
i = write_message(buf.buf, buf.len, todo_file, 0);
-- 
2.15.1.windows.2

[PATCH 3/5] sequencer: remove superfluous conditional

[Johannes Schindelin]

In a conditional block that is only reached when handling a TODO_REWORD
(as seen even from a 3-line context), there is absolutely no need to
nest another block under the identical condition.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 8e6b6289be6..38266c3c228 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1011,9 +1011,7 @@ static int do_pick_commit(enum todo_command command, 
struct commit *commit,
opts);
if (res || command != TODO_REWORD)
goto leave;
-   flags |= EDIT_MSG | AMEND_MSG;
-   if (command == TODO_REWORD)
-   flags |= VERIFY_MSG;
+   flags |= EDIT_MSG | AMEND_MSG | VERIFY_MSG;
msg_file = NULL;
goto fast_forward_edit;
}
-- 
2.15.1.windows.2

[PATCH 4/5] sequencer: report when noop has an argument

[Johannes Schindelin]

The noop command cannot accept any argument, but we never told the user
about any bogus argument. Fix that.

while at it, mention clearly when an argument is required but missing
(for commands *other* than noop).

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 13 +
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 38266c3c228..5632415ea2d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1259,18 +1259,23 @@ static int parse_insn_line(struct todo_item *item, 
const char *bol, char *eol)
if (i >= TODO_COMMENT)
return -1;
 
+   /* Eat up extra spaces/ tabs before object name */
+   padding = strspn(bol, " \t");
+   bol += padding;
+
if (item->command == TODO_NOOP) {
+   if (bol != eol)
+   return error(_("%s does not accept arguments: '%s'"),
+command_to_string(item->command), bol);
item->commit = NULL;
item->arg = bol;
item->arg_len = eol - bol;
return 0;
}
 
-   /* Eat up extra spaces/ tabs before object name */
-   padding = strspn(bol, " \t");
if (!padding)
-   return -1;
-   bol += padding;
+   return error(_("missing arguments for %s"),
+command_to_string(item->command));
 
if (item->command == TODO_EXEC) {
item->commit = NULL;
-- 
2.15.1.windows.2

[PATCH 2/5] sequencer: strip bogus LF at end of error messages

[Johannes Schindelin]

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 115085d39ca..8e6b6289be6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -491,7 +491,7 @@ static int is_index_unchanged(void)
struct commit *head_commit;
 
if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, _oid, NULL))
-   return error(_("could not resolve HEAD commit\n"));
+   return error(_("could not resolve HEAD commit"));
 
head_commit = lookup_commit(_oid);
 
@@ -511,7 +511,7 @@ static int is_index_unchanged(void)
 
if (!cache_tree_fully_valid(active_cache_tree))
if (cache_tree_update(_index, 0))
-   return error(_("unable to update cache tree\n"));
+   return error(_("unable to update cache tree"));
 
return !oidcmp(_cache_tree->oid,
   _commit->tree->object.oid);
@@ -697,12 +697,12 @@ static int is_original_commit_empty(struct commit *commit)
const struct object_id *ptree_oid;
 
if (parse_commit(commit))
-   return error(_("could not parse commit %s\n"),
+   return error(_("could not parse commit %s"),
 oid_to_hex(>object.oid));
if (commit->parents) {
struct commit *parent = commit->parents->item;
if (parse_commit(parent))
-   return error(_("could not parse parent commit %s\n"),
+   return error(_("could not parse parent commit %s"),
oid_to_hex(>object.oid));
ptree_oid = >tree->object.oid;
} else {
-- 
2.15.1.windows.2

[PATCH 0/5] A couple of sequencer cleanups

[Johannes Schindelin]

While working on patches to teach `git rebase -i` to recreate branch topology
properly, i.e. replace the design of `--preserve-merges` by something much
better, I stumbled across a couple of issues that I thought I should fix on the
way.

The patches are based on lb/rebase-i-short-command-names, mainly because the
new `--recreate-merges` patches benefit from the patch "rebase -i: update
functions to use a flags parameter", and I was too lazy to resolve the merge
conflicts while rebasing the patch "sequencer: do not invent whitespace when
transforming OIDs" to the current `master` branch.

Oh, and by the way, the `--recreate-merges` feature already works. I used it
to develop the patches themselves. I do not have time to pass one last time
over them, so they'll have to wait for next year to see the Git mailing list.
If anyone wants to have a look over them, play with them, or even wants to
review the patches:

https://github.com/git/git/compare/master...dscho:sequencer-shears


Johannes Schindelin (5):
  rebase: do not continue when the todo list generation failed
  sequencer: strip bogus LF at end of error messages
  sequencer: remove superfluous conditional
  sequencer: report when noop has an argument
  sequencer: do not invent whitespace when transforming OIDs

 git-rebase--interactive.sh |  3 ++-
 sequencer.c| 30 ++
 2 files changed, 20 insertions(+), 13 deletions(-)


base-commit: 1795993488bef1b48e4224db096e9d12df075db2
Based-On: lb/rebase-i-short-command-names at https://github.com/dscho/git
Fetch-Base-Via: git fetch https://github.com/dscho/git 
lb/rebase-i-short-command-names
Published-As: https://github.com/dscho/git/releases/tag/sequencer-cleanups-v1
Fetch-It-Via: git fetch https://github.com/dscho/git sequencer-cleanups-v1
-- 
2.15.1.windows.2

[PATCH 1/5] rebase: do not continue when the todo list generation failed

[Johannes Schindelin]

This is a *really* long-standing bug. As a matter of fact, this bug has
been with us from the very beginning of `rebase -i`: 1b1dce4bae7 (Teach
rebase an interactive mode, 2007-06-25), where the output of `rev-list`
was piped to `sed` (and any failure of the `rev-list` process would go
completely undetected).

Signed-off-by: Johannes Schindelin 
---
 git-rebase--interactive.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index e3f5a0abf3c..b7f95672bd9 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -893,7 +893,8 @@ fi
 if test t != "$preserve_merges"
 then
git rebase--helper --make-script ${keep_empty:+--keep-empty} \
-   $revisions ${restrict_revision+^$restrict_revision} >"$todo"
+   $revisions ${restrict_revision+^$restrict_revision} >"$todo" ||
+   die "$(gettext "Could not generate todo list")"
 else
format=$(git config --get rebase.instructionFormat)
# the 'rev-list .. | sed' requires %m to parse; the instruction 
requires %H to parse
-- 
2.15.1.windows.2

Re: [PATCH] Fix urlencode format string on signed char.

[Julien Dusser]

Thank you for review. I didn't find any other error.
Code in http.c:quote_ref_url() is almost the same but ch is a signed 
int, so there's no issue.


Le 22/12/2017 à 22:48, Junio C Hamano a écrit :

Julien Dusser  writes:


Git credential fails with special char in password.
remote: Invalid username or password.
fatal: Authentication failed for

File ~/.git-credential contains badly urlencoded characters
%ffXX%ffYY instead of %XX%YY.

Add a cast to an unsigned char to fix urlencode use of %02x
on a char.

Signed-off-by: Julien Dusser 
---
  strbuf.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/strbuf.c b/strbuf.c
index 323c49ceb..4d5a9ce55 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -658,7 +658,7 @@ static void strbuf_add_urlencode(struct strbuf *sb, const 
char *s, size_t len,
(!reserved && is_rfc3986_reserved(ch)))
strbuf_addch(sb, ch);
else
-   strbuf_addf(sb, "%%%02x", ch);
+   strbuf_addf(sb, "%%%02x", (unsigned char)ch);
}
  }


The issue is not limited to credential but anywhere where we need to
show a byte with hi-bit set, and it is obvious and straight-forward.

I briefly wondered if the data type for the strings involved in the
codepaths that reach this place should all be "uchar*" but it feels
strange to have "unsigned char *username" etc., and the signeness
matters only here, so the patch smells like the best one among other
possibilities.

Thanks.

[PATCH] oidmap: ensure map is initialized

[Brandon Williams]

Ensure that an oidmap is initialized before attempting to add, remove,
or retrieve an entry by simply performing the initialization step
before accessing the underlying hashmap.

Signed-off-by: Brandon Williams 
---
 oidmap.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/oidmap.c b/oidmap.c
index 6db4fffcd..d9fb19ba6 100644
--- a/oidmap.c
+++ b/oidmap.c
@@ -33,12 +33,19 @@ void oidmap_free(struct oidmap *map, int free_entries)
 
 void *oidmap_get(const struct oidmap *map, const struct object_id *key)
 {
+   if (!map->map.cmpfn)
+   return NULL;
+
return hashmap_get_from_hash(>map, hash(key), key);
 }
 
 void *oidmap_remove(struct oidmap *map, const struct object_id *key)
 {
struct hashmap_entry entry;
+
+   if (!map->map.cmpfn)
+   oidmap_init(map, 0);
+
hashmap_entry_init(, hash(key));
return hashmap_remove(>map, , key);
 }
@@ -46,6 +53,10 @@ void *oidmap_remove(struct oidmap *map, const struct 
object_id *key)
 void *oidmap_put(struct oidmap *map, void *entry)
 {
struct oidmap_entry *to_put = entry;
+
+   if (!map->map.cmpfn)
+   oidmap_init(map, 0);
+
hashmap_entry_init(_put->internal_entry, hash(_put->oid));
return hashmap_put(>map, to_put);
 }
-- 
2.15.1.620.gb9897f4670-goog

Re: [PATCHv5 7/7] builtin/describe.c: describe a blob

[Junio C Hamano]

Stefan Beller  writes:

> On Tue, Dec 19, 2017 at 11:22 AM, Junio C Hamano  wrote:
>> I had to squash in the following to make 'pu' pass under
>> gettext-poison build.  Is this ready for 'next' otherwise?
>
> I saw that in pu, thanks for squashing. I should have spoken
> up earlier confirming it.

So is this ready for 'next' if it is squashed?

>> With the "log --find-object" thing, it may be that this no longer is
>> needed, but then again we haven't done anything with the other
>> Jonathan's idea to unify the --find-object thing into the --pickaxe
>> framework.
>
> I'll look into that after I finish looking at a submodule related bug.

Thanks.

>> It seems that we tend to open and then abandon new interests without
>> seeing them through completion, leaving too many loose ends untied.
>> This has to stop.
>
> I'll try to find my balance again.

Sorry, but this was not really about you, but was more about me.  

I probably should more aggressively drop a topic that stalled and
also merge a topic that reached the point of diminishing returns to
'next'.

Re: Usability outrage as far as I am concerned

[Jacob Keller]

On Fri, Dec 22, 2017 at 7:57 AM, Cristian Achim  wrote:
>> Can you show the output of "git remote"
>
> # in usb_subfolder
> $git remote
> origin
> $
>
> #in home_subfolder
> $git remote
> $
>

With the -v switch you can see where each remote points to (tho your
home local repo has no remote which is fine).

>> and also
>> clearly explain with details the layout of what the folders are and
>> what is or is not a repository?
>
> Take the following update into consideration and then reread my first
> email hopefully with improved clarity:
>
> 'home_subfolder' is the path on disk inside my user account home
> folder in the 'home' root folder to the initial repo from which I
> meant to do a backup.
>
> 'usb_subfolder' is the path on disk in the 'media' root folder to the
> initial empty folder into which I wanted to do the backup above that
> points into a usb stick I mounted in the default Kubuntu KDE file
> manager way of mounting usb stick folder hierarchies.
>
> Current situation is that 'git log' in both home_subfolder and
> usb_subfolder show the same hash with only one branch in both. From
> usb_subfolder 'git pull home_subfolder' is broken as in the original
> message.

Ok. So you have a repository inside your home directory which you wish
to copy into the USB stick?

So what steps did you take to setup the repository usb_subfolder initially?

You're basically trying to create a backup copy of what's in
home_subfolder into your USB stick?

If you're confident that home_subfolder is accurate right now, (ie:
you inspect its contents with git log, git status, and regaulr
commands to check that everything is as you expect), here's what I
would do:

cd to your usb stick, then run
git clone /path/to/home_subfolder

this will create an initial clone.

If you wish to update it later, you can mount hte usb stick, and then
just run git pull from inside the new subfolder. Note that you do
*not* run "git pull home_subfolder", as git pull expects the name of a
remote, which in this case is just origin (since the default remote
name you clone from is origin)

I'm still not certain what state you got in, but I believe based on
your commands that the home_subfolder is fine, and you somehow
incorrectly setup the usb_subfolder.

Thanks,
Jake

PS/Tangent:

If you never need the checked out files on the USB disk, and only wish
to keep history saved, then you can actually do "git clone --mirror
" in order to make a bare copy which is a complete mirror of all
refs in the original repository.

Then you can update it with just git fetch, or git remote update.
(tho, keep in mind this clone would not have any working tree, but
merely a bare repository contents). (You can, ofcourse, recover the
files by simply cloning to somewhere else, or adding a new work tree
or similar.

Re: [PATCH v2 2/2] commit: add support for --fixup -m""

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> I don't agree that git as a tool should be so opinionated. You can edit
> these --fixup messages right now with --edit, and I do. That it doesn't
> work with -m"" as it should is a longstanding UI wart.

I think you missed the point.

I was expressing my opinion, not an opinion of Git as a tool, that I
think one of these two "use case" scenario was a bad way not to be
encouraged.

That is totally different from allowing --fixup and -m working
together.  That is a good thing that helps the other "good" use case.

Re: [PATCH v2 2/2] commit: add support for --fixup -m""

[Ævar Arnfjörð Bjarmason]

On Fri, Dec 22 2017, Junio C. Hamano jotted:

> Ævar Arnfjörð Bjarmason   writes:
>
>> Those options could also support combining with -m, but given what
>> they do I can't think of a good use-case for doing that, so I have not
>> made the more invasive change of splitting up the logic in commit.c to
>> first act on those, and then on -m options.
>>
>> 1. d71b8ba7c9 ("commit: --fixup option for use with rebase
>>--autosquash", 2010-11-02)
>
> To be fair, when "rebase --autosquash -i" is run (which is why you
> would use --fixup in the first place), the log message of the fixup
> one is used only for locating which one is to be corrected, and the
> contents of the log message is discarded.  So "given what it does",
> I can't think of a good use-case for using --fixup and -m together,
> either.  So "nobody immediately thought of it when it was added" is
> certainly not a reason to later make the combination possible.
>
> But I personally am moderately negative on one of these two imagined
> use cases.
>
>> Add support for supplying the -m option with --fixup. Doing so has
>> errored out ever since --fixup was introduced. Before this, the only
>> way to amend the fixup message while committing was to use --edit and
>> amend it in the editor.
>>
>> The use-case for this feature is one of:
>>
>>  * Leaving a quick note to self when creating a --fixup commit when
>>it's not self-evident why the commit should be squashed without a
>>note into another one.
>
> This is probably OK.
>
>>  * (Ab)using the --fixup feature to "fix up" commits that have already
>>been pushed to a branch that doesn't allow non-fast-forwards,
>>i.e. just noting "this should have been part of that other commit",
>>and if the history ever got rewritten in the future the two should
>>be combined.
>
> This has a smell of the tail wagging the dog.
>
> Perhaps your editor does not have a good integration with external
> commands to allow you to insert a single-liner output from
>
> git show --date=short -s --pretty='format:%h ("%s", %ad)' "$1"

Since this use-case is talking about pushing a fixup for an already
pushed commit, this is the first thing I put in -m"..." to reduce
ambiguity.

> and that is what you are abusing --fixup for?
>
> It is simply bad practice to leave a log entry that begins with
> !fixup marker that would confuse automated tools like "rebase -i"
> machinery on a commit that you have no intention of squashing into
> another, as it invites mistakes.

"if the history ever got rewritten in the future the two should be
combined"

So it's still the intent to squash these, it's just not being done right
now, and even if it never happens it'll be easy to glance at the
relevant commits in log --oneline.

> I do agree with the scenario where you would wish you could take
> back an earlier mistake but you cannot.  But the log for such a
> follow-up fix should be written just like any other follow-up fix
> commit, i.e. describe what was wrong and how the wrongness is
> corrected with the follow-up change.  What was wrong in "which
> commit" is of course important part, but it is a relatively small
> part.

I don't agree that git as a tool should be so opinionated. You can edit
these --fixup messages right now with --edit, and I do. That it doesn't
work with -m"" as it should is a longstanding UI wart.

Tools should be naturally composable without needless arbitrary
limitations.

Re: [PATCH] Fix urlencode format string on signed char.

[Junio C Hamano]

Julien Dusser  writes:

> Git credential fails with special char in password.
> remote: Invalid username or password.
> fatal: Authentication failed for
>
> File ~/.git-credential contains badly urlencoded characters
> %ffXX%ffYY instead of %XX%YY.
>
> Add a cast to an unsigned char to fix urlencode use of %02x
> on a char.
>
> Signed-off-by: Julien Dusser 
> ---
>  strbuf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/strbuf.c b/strbuf.c
> index 323c49ceb..4d5a9ce55 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -658,7 +658,7 @@ static void strbuf_add_urlencode(struct strbuf *sb, const 
> char *s, size_t len,
>   (!reserved && is_rfc3986_reserved(ch)))
>   strbuf_addch(sb, ch);
>   else
> - strbuf_addf(sb, "%%%02x", ch);
> + strbuf_addf(sb, "%%%02x", (unsigned char)ch);
>   }
>  }

The issue is not limited to credential but anywhere where we need to
show a byte with hi-bit set, and it is obvious and straight-forward.

I briefly wondered if the data type for the strings involved in the
codepaths that reach this place should all be "uchar*" but it feels
strange to have "unsigned char *username" etc., and the signeness
matters only here, so the patch smells like the best one among other
possibilities.

Thanks.

Re: [PATCH] sequencer: assign only free()able strings to gpg_sign

[Junio C Hamano]

"phillip.w...@talktalk.net"  writes:

>>Original Message
>>From: johannes.schinde...@gmx.de
>>Date: 22/12/2017 11:50 
>>To: 
>>Cc: "Junio C Hamano", "Phillip Wood" w...@dunelm.org.uk>, "Kaartic Sivaraam"
>>Subj: [PATCH] sequencer: assign only free()able strings to gpg_sign
>>
>>The gpg_sign member of the replay_opts structure is of type `char *`,
>>meaning that the sequencer deems the string to which gpg_sign points 
> to
>>be under its custody, i.e. it needs to be free()d by the sequencer.
>>
>>Therefore, let's only assign malloc()ed buffers to it.
>>
>>Reported-by: Kaartic Sivaraam 
>>Signed-off-by: Johannes Schindelin 
>>---
>>
>>  Phillip, if you want to squash these changes into your patches,
>>  I'd totally fine with that.
>>
>
> Hi Johannes, thanks for putting this together, the patch it fixes is 
> already in next so I think it'd be best to leave this one separate. I 
> wonder if it would be worth adding another test, see below.

Thanks, both.  Let's queue this on top as a fix-up.

Re: [PATCH v2 2/2] commit: add support for --fixup -m""

[Junio C Hamano]

Junio C Hamano  writes:

> ...  So "nobody immediately thought of it when it was added" is
> certainly not a reason to later make the combination possible.

Oops, not a reason to later "_not_ to" make an update, of course.

Re: [PATCH v2 2/2] commit: add support for --fixup -m""

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> Those options could also support combining with -m, but given what
> they do I can't think of a good use-case for doing that, so I have not
> made the more invasive change of splitting up the logic in commit.c to
> first act on those, and then on -m options.
>
> 1. d71b8ba7c9 ("commit: --fixup option for use with rebase
>--autosquash", 2010-11-02)

To be fair, when "rebase --autosquash -i" is run (which is why you
would use --fixup in the first place), the log message of the fixup
one is used only for locating which one is to be corrected, and the
contents of the log message is discarded.  So "given what it does",
I can't think of a good use-case for using --fixup and -m together,
either.  So "nobody immediately thought of it when it was added" is
certainly not a reason to later make the combination possible.

But I personally am moderately negative on one of these two imagined
use cases.

> Add support for supplying the -m option with --fixup. Doing so has
> errored out ever since --fixup was introduced. Before this, the only
> way to amend the fixup message while committing was to use --edit and
> amend it in the editor.
>
> The use-case for this feature is one of:
>
>  * Leaving a quick note to self when creating a --fixup commit when
>it's not self-evident why the commit should be squashed without a
>note into another one.

This is probably OK.

>  * (Ab)using the --fixup feature to "fix up" commits that have already
>been pushed to a branch that doesn't allow non-fast-forwards,
>i.e. just noting "this should have been part of that other commit",
>and if the history ever got rewritten in the future the two should
>be combined.

This has a smell of the tail wagging the dog.

Perhaps your editor does not have a good integration with external
commands to allow you to insert a single-liner output from

git show --date=short -s --pretty='format:%h ("%s", %ad)' "$1"

and that is what you are abusing --fixup for?  

It is simply bad practice to leave a log entry that begins with
!fixup marker that would confuse automated tools like "rebase -i"
machinery on a commit that you have no intention of squashing into
another, as it invites mistakes.  

I do agree with the scenario where you would wish you could take
back an earlier mistake but you cannot.  But the log for such a
follow-up fix should be written just like any other follow-up fix
commit, i.e. describe what was wrong and how the wrongness is
corrected with the follow-up change.  What was wrong in "which
commit" is of course important part, but it is a relatively small
part.

Re: [PATCH v3 1/1] check-non-portable-shell.pl: Quoted `wc -l` is not portable

[Junio C Hamano]

tbo...@web.de writes:

>
> Reviewed-by: Johannes Schindelin 
> Signed-off-by: Torsten Bögershausen 

I'll flip these and add a helped-by to credit Eric.

check-non-portable-shell.pl: `wc -l` may have leading WS

Test scripts count number of lines in an output and check it againt
its expectation.  fb3340a6 ("test-lib: introduce test_line_count to
measure files", 2010-10-31) introduced a helper to show a failure in
such a test in a more readable way than comparing `wc -l` output with
a number.

Besides, on some platforms, "$(wc -l  @@ -21,6 +21,7 @@ while (<>) {
>   /^\s*declare\s+/ and err 'arrays/declare not portable';
>   /^\s*[^#]\s*which\s/ and err 'which is not portable (please use type)';
>   /\btest\s+[^=]*==/ and err '"test a == b" is not portable (please use 
> =)';
> + /\bwc -l.*"\s*=/ and err '`"$(wc -l)"` is not portable (please use 
> test_line_count)';
>   /\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable 
> (please use FOO=bar && export FOO)';
>   # this resets our $. for each file
>   close ARGV if eof;

Thanks.  The dq before \s*= is rather cute ;-)

Re: [PATCH] config.txt: Document behavior of backslashes in subsections

[Junio C Hamano]

Dave Borowitz  writes:

> Unrecognized escape sequences are invalid in values:
>
>   $ git config -f - --list <   [foo]
> bar = "\t\\\y\"\u"
>   EOF
>   fatal: bad config line 2 in standard input
>
> But in subsection names, the backslash is simply dropped if the
> following character does not produce a recognized escape sequence:
>
>   $ git config -f - --list <   [foo "\t\\\y\"\u"]
> bar = baz
>   EOF
>   foo.t\y"u.bar=baz
>
> Although it would be nice for subsection names and values to have
> consistent behavior, changing the behavior for subsection names is a
> nonstarter since it would cause existing, valid config files to
> suddenly be interpreted differently.
>
> Signed-off-by: Dave Borowitz 
> ---
>  Documentation/config.txt | 12 +++-
>  1 file changed, 7 insertions(+), 5 deletions(-)

Thanks.

>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index b18c0f97fe..f772186c44 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -41,11 +41,13 @@ in the section header, like in the example below:
>  
>  
>  Subsection names are case sensitive and can contain any characters except
> -newline (doublequote `"` and backslash can be included by escaping them
> -as `\"` and `\\`, respectively).  Section headers cannot span multiple
> -lines.  Variables may belong directly to a section or to a given subsection.
> -You can have `[section]` if you have `[section "subsection"]`, but you
> -don't need to.
> +newline and the null byte. Doublequote `"` and backslash can be included
> +by escaping them as `\"` and `\\`, respectively. Backslashes preceding
> +other characters are dropped when reading; for example, `\t` is read as
> +`t` and `\0` is read as `0` Section headers cannot span multiple lines.
> +Variables may belong directly to a section or to a given subsection. You
> +can have `[section]` if you have `[section "subsection"]`, but you don't
> +need to.
>  
>  There is also a deprecated `[section.subsection]` syntax. With this
>  syntax, the subsection name is converted to lower-case and is also

Re: [PATCH 2/3] move index_has_changes() from builtin/am.c to merge.c for reuse

[Junio C Hamano]

Elijah Newren  writes:

> On Thu, Dec 21, 2017 at 11:19 AM, Elijah Newren  wrote:
>> index_has_changes() is a function we want to reuse outside of just am,
>> making it also available for merge-recursive and merge-ort.
>>
>> Signed-off-by: Elijah Newren 
>> ---
>
> Note: These patches built on master, and merge cleanly with next and
> pu.  However, this patch has a minor conflict with maint.  If you'd
> prefer a version that applies on top of maint, let me know and I'll
> resubmit.

I think I managed to create two topics, one that is with these three
patches (2/3 backported) on top of maint and the other one merges
the former on top of master.  Please see if you found a mismerge
when I push the results out.

Thanks.

Re: [PATCH v2 0/2] support -m"" combined with commit --fixup

[Ævar Arnfjörð Bjarmason]

On Fri, Dec 22 2017, Eric Sunshine jotted:

> On Fri, Dec 22, 2017 at 11:00 AM, Ævar Arnfjörð Bjarmason
>  wrote:
>> Here's a hopefully ready to apply v2 incorporating feedback from Eric
>> (thanks!). A tbdiff with v1 follows below.
>>
>> Ævar Arnfjörð Bjarmason (2):
>>   commit doc: document that -c, -C, -F and --fixup with -m error
>>   commit: add support for --fixup  -m""
>
> Patch 2/2 doesn't seem to have made it to the list...

Oops, here it comes.

>> 2: bd78a211ed ! 2: 780de6e042 commit: add support for --fixup  
>> -m""
>> @@ -22,6 +22,21 @@
>> In such a case you might want to leave a small message,
>> e.g. "forgot this part, which broke XYZ".
>>
>> +With this, --fixup  -m"More" -m"Details" will result in a
>> +commit message like:
>> +
>> +!fixup >
>> +
>> +More
>> +
>> +Details
>> +
>> +The reason the test being added here seems to squash "More" at the 
>> end
>> +of the subject line of the commit being fixed up is because the test
>> +code is using "%s%b" so the body immediately follows the subject, 
>> it's
>> +not a bug in this code, and other tests t7500-commit.sh do the same
>> +thing.
>
> Did you also intend to mention something about --edit still working
> with -m? (Or do we assume that people will understand automatically
> that it does?)

I thought it was clear enough since it works with --fixup now and with
everything else, and the commit message was already getting long
enough...

[PATCH v2 2/2] commit: add support for --fixup -m""

[Ævar Arnfjörð Bjarmason]

Add support for supplying the -m option with --fixup. Doing so has
errored out ever since --fixup was introduced. Before this, the only
way to amend the fixup message while committing was to use --edit and
amend it in the editor.

The use-case for this feature is one of:

 * Leaving a quick note to self when creating a --fixup commit when
   it's not self-evident why the commit should be squashed without a
   note into another one.

 * (Ab)using the --fixup feature to "fix up" commits that have already
   been pushed to a branch that doesn't allow non-fast-forwards,
   i.e. just noting "this should have been part of that other commit",
   and if the history ever got rewritten in the future the two should
   be combined.

   In such a case you might want to leave a small message,
   e.g. "forgot this part, which broke XYZ".

With this, --fixup  -m"More" -m"Details" will result in a
commit message like:

!fixup >

More

Details

The reason the test being added here seems to squash "More" at the end
of the subject line of the commit being fixed up is because the test
code is using "%s%b" so the body immediately follows the subject, it's
not a bug in this code, and other tests t7500-commit.sh do the same
thing.

When the --fixup option was initially added the "Option -m cannot be
combined" error was expanded from -c, -C and -F to also include
--fixup[1]

Those options could also support combining with -m, but given what
they do I can't think of a good use-case for doing that, so I have not
made the more invasive change of splitting up the logic in commit.c to
first act on those, and then on -m options.

1. d71b8ba7c9 ("commit: --fixup option for use with rebase
   --autosquash", 2010-11-02)

Helped-by: Eric Sunshine 
Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/git-commit.txt | 3 +--
 builtin/commit.c | 8 +---
 t/t7500-commit.sh| 9 -
 3 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 3fbb7352bc..f970a43422 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -145,8 +145,7 @@ OPTIONS
If multiple `-m` options are given, their values are
concatenated as separate paragraphs.
 +
-The `-m` option is mutually exclusive with `-c`, `-C`, `-F`, and
-`--fixup`.
+The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`.
 
 -t ::
 --template=::
diff --git a/builtin/commit.c b/builtin/commit.c
index 8a87701414..4e68394391 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -701,7 +701,7 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
}
}
 
-   if (have_option_m) {
+   if (have_option_m && !fixup_message) {
strbuf_addbuf(, );
hook_arg1 = "message";
} else if (logfile && !strcmp(logfile, "-")) {
@@ -731,6 +731,8 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
ctx.output_encoding = get_commit_output_encoding();
format_commit_message(commit, "fixup! %s\n\n",
  , );
+   if (have_option_m)
+   strbuf_addbuf(, );
hook_arg1 = "message";
} else if (!stat(git_path_merge_msg(), )) {
/*
@@ -1197,8 +1199,8 @@ static int parse_and_validate_options(int argc, const 
char *argv[],
f++;
if (f > 1)
die(_("Only one of -c/-C/-F/--fixup can be used."));
-   if (have_option_m && f > 0)
-   die((_("Option -m cannot be combined with -c/-C/-F/--fixup.")));
+   if (have_option_m && (edit_message || use_message || logfile))
+   die((_("Option -m cannot be combined with -c/-C/-F.")));
if (f || have_option_m)
template_file = NULL;
if (edit_message)
diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh
index 5739d3ed23..2d95778b74 100755
--- a/t/t7500-commit.sh
+++ b/t/t7500-commit.sh
@@ -272,6 +272,14 @@ test_expect_success 'commit --fixup provides correct 
one-line commit message' '
commit_msg_is "fixup! target message subject line"
 '
 
+test_expect_success 'commit --fixup -m"something" -m"extra"' '
+   commit_for_rebase_autosquash_setup &&
+   git commit --fixup HEAD~1 -m"something" -m"extra" &&
+   commit_msg_is "fixup! target message subject linesomething
+
+extra"
+'
+
 test_expect_success 'commit --squash works with -F' '
commit_for_rebase_autosquash_setup &&
echo "log message from file" >msgfile &&
@@ -325,7 +333,6 @@ test_expect_success 'invalid message options when using 
--fixup' '
test_must_fail git commit --fixup HEAD~1 --squash HEAD~2 &&
test_must_fail git commit --fixup HEAD~1 -C HEAD~2 &&
test_must_fail git commit --fixup HEAD~1 -c HEAD~2 &&
-   test_must_fail git commit 

Re: [PATCH 3/3] merge-recursive: Avoid incorporating uncommitted changes in a merge

[Junio C Hamano]

Elijah Newren  writes:

> builtin/merge.c contains this important requirement for merge strategies:
>   /*
>* At this point, we need a real merge.  No matter what strategy
>* we use, it would operate on the index, possibly affecting the
>* working tree, and when resolved cleanly, have the desired
>* tree in the index -- this means that the index must be in
>* sync with the head commit.  The strategies are responsible
>* to ensure this.
>*/
>
> merge-recursive does not do this check directly, instead it relies on
> unpack_trees() to do it.  However, merge_trees() has a special check for
> the merge branch exactly matching the merge base; when it detects that
> situation, it returns early without calling unpack_trees(), because it
> knows that the HEAD commit already has the correct result.  Unfortunately,
> it didn't check that the index matched HEAD, so after it returned, the
> outer logic ended up creating a merge commit that included something
> other than HEAD.

Good.

I actually was imagining that you would shoot for creating an empty
commit and leaving a working tree and the index that are both dirty,
but I do not think it is worth the effort.  Besides, "you have to
start from a clean index" is a much simpler rule to explain than
with "unless the resulting tree is the same as HEAD", especially
when that "unless" is highly unlikely to happen anyway.

Thanks.

>
> Signed-off-by: Elijah Newren 
> ---
>  merge-recursive.c| 7 +++
>  t/t6044-merge-unrelated-index-changes.sh | 2 +-
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 2ecf495cc2..780f81a8bd 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1952,6 +1952,13 @@ int merge_trees(struct merge_options *o,
>   }
>  
>   if (oid_eq(>object.oid, >object.oid)) {
> + struct strbuf sb = STRBUF_INIT;
> +
> + if (index_has_changes()) {
> + err(o, _("Dirty index: cannot merge (dirty: %s)"),
> + sb.buf);
> + return 0;
> + }
>   output(o, 0, _("Already up to date!"));
>   *result = head;
>   return 1;
> diff --git a/t/t6044-merge-unrelated-index-changes.sh 
> b/t/t6044-merge-unrelated-index-changes.sh
> index 5e472be92b..23b86fb977 100755
> --- a/t/t6044-merge-unrelated-index-changes.sh
> +++ b/t/t6044-merge-unrelated-index-changes.sh
> @@ -112,7 +112,7 @@ test_expect_success 'recursive' '
>   test_must_fail git merge -s recursive C^0
>  '
>  
> -test_expect_failure 'recursive, when merge branch matches merge base' '
> +test_expect_success 'recursive, when merge branch matches merge base' '
>   git reset --hard &&
>   git checkout B^0 &&

Re: [PATCH] oidmap.h: strongly discourage using OIDMAP_INIT directly

[Junio C Hamano]

Brandon Williams  writes:

>> -#define OIDMAP_INIT { { NULL } }
>> +/*
>> + * This macro initializes the data structure only incompletely, just enough
>> + * to call oidmap_get() on an empty map. Use oidmap_init() instead.
>> + */
>> +#define OIDMAP_INIT_INCOMPLETELY { { NULL } }
>
> This is one way of approaching the problem.  Couldn't we also take the
> approach like we have with oidset and ensure that when oidmap_get() or
> _put() are called that if the oidmap isn't initialized, we simply do
> that then?

Hmph.  Can you show how the alternative code would look like?

Re: [PATCH] send-pack: use internal argv_array of struct child_process

[Junio C Hamano]

Stefan Beller  writes:

>> +   argv_array_push(, "pack-objects");
>> +   argv_array_push(, "--all-progress-implied");
>> +   argv_array_push(, "--revs");
>> +   argv_array_push(, "--stdout");
>
> (useless nit of the day, no need to resend:)
> These four statements could be done as pushl(a,b,c, NULL);
> but it would not differ in readability, so I guess either is fine.

Yup.  Shorter is not necessarily better.  If we anticipate that we
will need to update the set of options later (and I suspect we may
be doing so soon, given what JeffH and JTan are working on
recently), one option (or option-argument pair) per line is a more
preferrable form from the maintenance point of view.

Re: [PATCH] t/helper/test-lazy-name-hash: fix compilation

[Junio C Hamano]

Stefan Beller  writes:

>> Heh; I do not think there particularly is much difference between
>> stricter flags and DEVELOPER flags, but I would rather not lose the
>> removal of duplicated 'today' I did while I queued the previous one
>> ;-)
>
>
> I did not realize it was queued anywhere, please ignore then.

I just did s/stricter/DEVELOPER/ on the one that have been queued.
Thanks.

Re: [PATCH 4/4] submodule: submodule_move_head omits old argument in forced case

[Junio C Hamano]

Stefan Beller  writes:

> When using hard reset or forced checkout with the option to recurse into
> submodules, the submodules need to be reset, too.
>
> It turns out that we need to omit the duplicate old argument to read-tree
> in all forced cases to omit the 2 way merge and use the more assertive
> behavior of reading the specific new tree into the index and updating
> the working tree.

The phrase "the more assertive" made me imagine something like
"reset --hard", which resurrect lost paths and also get rid of added
paths.  "reading the specific new tree into the index" smells more
like "checkout $tree-ish $paths" that has an overlay semantics, that
resurrects lost paths but does not get rid of added paths.

Perhaps not just "rm sub1/file1" but also add a new file that is not
in HEAD to ensure that it will be blown away when $command is run
to ensure that we got the distinction between the two right?

>
> Signed-off-by: Stefan Beller 
> ---
>  submodule.c   |  4 +++-
>  t/lib-submodule-update.sh | 11 +++
>  2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/submodule.c b/submodule.c
> index fa25888783..db0f7ac51e 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1653,7 +1653,9 @@ int submodule_move_head(const char *path,
>   else
>   argv_array_push(, "-m");
>  
> - argv_array_push(, old ? old : EMPTY_TREE_SHA1_HEX);
> + if (!(flags & SUBMODULE_MOVE_HEAD_FORCE))
> + argv_array_push(, old ? old : EMPTY_TREE_SHA1_HEX);
> +
>   argv_array_push(, new ? new : EMPTY_TREE_SHA1_HEX);
>  
>   if (run_command()) {
> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
> index fb0173ea87..380ef4b4ae 100755
> --- a/t/lib-submodule-update.sh
> +++ b/t/lib-submodule-update.sh
> @@ -1015,4 +1015,15 @@ test_submodule_forced_switch_recursing_with_args () {
>   test_submodule_content sub1 origin/modify_sub1
>   )
>   '
> +
> + test_expect_success "$command: changed submodule worktree is reset" '
> + prolog &&
> + reset_work_tree_to_interested add_sub1 &&
> + (
> + cd submodule_update &&
> + rm sub1/file1 &&
> + $command HEAD &&
> + test_path_is_file sub1/file1
> + )
> + '
>  }

Re: [PATCH 3/4] unpack-trees: Fix same() for submodules

[Junio C Hamano]

Stefan Beller  writes:

> The function same(a, b) is used to check if two entries a and b are the
> same.  As the index contains the staged files the same() function can be
> used to check if files between a given revision and the index are the same.
>
> In case of submodules, the gitlink entry is showing up as not modified
> despite potential changes inside the submodule.
>
> Fix the function to examine submodules by looking inside the submodule.
> This patch alone doesn't affect any correctness garantuees, but in

guarantees?  I somehow misread this as orangutan ;-)

> combination with the next patch this fixes the new test introduced
> earlier in this series.
>
> This may be a slight performance regression as now we have to
> inspect any submodule thouroughly.
>
> Signed-off-by: Stefan Beller 
> ---
>  unpack-trees.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index bf8b602901..4d839e8edb 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -1427,6 +1427,8 @@ static int same(const struct cache_entry *a, const 
> struct cache_entry *b)
>   return 1;
>   if ((a->ce_flags | b->ce_flags) & CE_CONFLICTED)
>   return 0;
> + if (S_ISGITLINK(b->ce_mode) && should_update_submodules())
> + return !oidcmp(>oid, >oid) && 
> !is_submodule_modified(b->name, 0);

Isn't this is working at the wrong level?

It is technically correct to say "the function is used to check if
two entries a and be are the same", but that statement misses a more
important aspect of the function.  It is asked if these two things
are the same and does not get involved in seeing if the working tree
is dirty with respect to (one of) the entries it was passed.

For example, a virtual merge in a recursive merge would not (and
should not) care if local changes exist in the working tree, and
this function does not get any hint if it is to
look at the working tree to tell such a case and the outermost merge
apart.

Somebody has to be careful not to stomp on local changes in the
outmost merge in a recursive merge, but I do not think this function
is designed to be the place to do so.  Isn't a submodule with
"potential changes" the same way?



>   return a->ce_mode == b->ce_mode &&
>  !oidcmp(>oid, >oid);
>  }

Re: [PATCH v2 0/2] support -m"" combined with commit --fixup

[Eric Sunshine]

On Fri, Dec 22, 2017 at 11:00 AM, Ævar Arnfjörð Bjarmason
 wrote:
> Here's a hopefully ready to apply v2 incorporating feedback from Eric
> (thanks!). A tbdiff with v1 follows below.
>
> Ævar Arnfjörð Bjarmason (2):
>   commit doc: document that -c, -C, -F and --fixup with -m error
>   commit: add support for --fixup  -m""

Patch 2/2 doesn't seem to have made it to the list...

> 2: bd78a211ed ! 2: 780de6e042 commit: add support for --fixup  
> -m""
> @@ -22,6 +22,21 @@
> In such a case you might want to leave a small message,
> e.g. "forgot this part, which broke XYZ".
>
> +With this, --fixup  -m"More" -m"Details" will result in a
> +commit message like:
> +
> +!fixup >
> +
> +More
> +
> +Details
> +
> +The reason the test being added here seems to squash "More" at the 
> end
> +of the subject line of the commit being fixed up is because the test
> +code is using "%s%b" so the body immediately follows the subject, 
> it's
> +not a bug in this code, and other tests t7500-commit.sh do the same
> +thing.

Did you also intend to mention something about --edit still working
with -m? (Or do we assume that people will understand automatically
that it does?)

Re: [PATCH 2/5] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules

[Junio C Hamano]

Stefan Beller  writes:

> It turns out that this buggy test passed due to the buggy implementation,
> which will soon be corrected.  Let's fix the test first.

Please clarify "buggy test".  Without the original discussion (I am
imagining there is something that happened on the list recently),
here is my guess:

We tried to make sure that an ignored file is $distimmed by
$gostak, but forgot to tell the exclude mechanism that the path
'sub1'ignored' we use for the test is actually ignored, so the
fact that the file was not $distimmed when $gostak did its thing
meant nothing---mark any path whose name is 'ignored' as ignored
to really test the condition we want to test.

but I do not exactly know what verb to replace $distim and what noun
to replace $gostak above ;-)

Also, wouldn't this expose a bug in the implementation if that is
the case?

Thanks.

>
> Signed-off-by: Stefan Beller 
> ---
>  t/lib-submodule-update.sh | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
> index d7699046f6..fb0173ea87 100755
> --- a/t/lib-submodule-update.sh
> +++ b/t/lib-submodule-update.sh
> @@ -885,6 +885,7 @@ test_submodule_switch_recursing_with_args () {
>   (
>   cd submodule_update &&
>   git branch -t replace_sub1_with_file 
> origin/replace_sub1_with_file &&
> + echo ignored >.git/modules/sub1/info/exclude &&
>   : >sub1/ignored &&
>   $command replace_sub1_with_file &&
>   test_superproject_content origin/replace_sub1_with_file 
> &&

Re: [PATCH v6] Makefile: replace perl/Makefile.PL with simple make rules

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

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

Thanks, but I thought the patch was already in 'next' for a week or
more and it's time to refine incrementally.

Here is the difference as I see between what we already have and
this update, and a proposed summary.

-- >8 --
From: Ævar Arnfjörð Bjarmason 
Subject: perl: avoid *.pmc and fix Error.pm further

The previous round tried to use *.pmc files but it confused RPM
dependency analysis on some distros.  Install them as plain
vanilla *.pm files instead.

Also "local @_" construct did not properly work when goto 
is used until recent versions of Perl.  Avoid it (and we do not
need to localize it here anyway).


---
diff --git a/Makefile b/Makefile
index ba6607b7e7..5c73cd208a 100644
--- a/Makefile
+++ b/Makefile
@@ -2274,14 +2274,14 @@ endif
 po/build/locale/%/LC_MESSAGES/git.mo: po/%.po
$(QUIET_MSGFMT)mkdir -p $(dir $@) && $(MSGFMT) -o $@ $<
 
-PMFILES := $(wildcard perl/*.pm perl/*/*.pm perl/*/*/*.pm perl/*/*/*/*.pm)
-PMCFILES := $(patsubst perl/%.pm,perl/build/lib/%.pmc,$(PMFILES))
+LIB_PERL := $(wildcard perl/Git.pm perl/Git/*.pm perl/Git/*/*.pm 
perl/Git/*/*/*.pm)
+LIB_PERL_GEN := $(patsubst perl/%.pm,perl/build/lib/%.pm,$(LIB_PERL))
 
 ifndef NO_PERL
-all:: $(PMCFILES)
+all:: $(LIB_PERL_GEN)
 endif
 
-perl/build/lib/%.pmc: perl/%.pm
+perl/build/lib/%.pm: perl/%.pm
$(QUIET_GEN)mkdir -p $(dir $@) && \
sed -e 's|@@LOCALEDIR@@|$(localedir_SQ)|g' < $< > $@
 
diff --git a/perl/Git/Error.pm b/perl/Git/Error.pm
index 5874672150..09bbc97390 100644
--- a/perl/Git/Error.pm
+++ b/perl/Git/Error.pm
@@ -39,7 +39,7 @@ sub import {
require Error;
 };
 
-local @_ = ($caller, @_);
+unshift @_, $caller;
 goto ::import;
 }
 

Re: Fetching commit instead of ref

[Junio C Hamano]

"Carlsson, Magnus"  writes:

> $ git fetch subrepo 
> 50f730db793e0733b159326c5a3e78fd48cedfec:refs/remote/subrepo/foo-commit
> remote: Counting objects: 2311, done.
> remote: Total 2311 (delta 0), reused 0 (delta 0), pack-reused 2311
> -- >>> Receiving objects: 100% (2311/2311), 703.64 KiB | 0 bytes/s, done.
> Resolving deltas: 100% (1174/1174), done.
> error: Server does not allow request for unadvertised object 
> 50f730db793e0733b159326c5a3e78fd48cedfec

Unless that "fetch" is doing a recursive fetch from another
repository (which causes the counting to be shown) or something
silly, the above makes it look like that the server is broken.

A quick test locally does not seem to show such a behaviour, but I
do not enable pack bitmaps and I know github does at least for some
repositories, so it is possible there is a problem somewhere in that
codepath.

Re: [PATCH] sequencer: assign only free()able strings to gpg_sign

[Junio C Hamano]

Johannes Schindelin  writes:

>> I thought the bug could be triggered when commit.gpgsign was true and 
>> it was not overriden on the commandline, is it worth adding a test for 
>> that?
>
> ... If we want to verify that the
> gpg_sign is correctly allocated before it is free()d, then the test case I
> added *already* covers it,...

It depends on what we are testing, how we anticipate this code will
be broken by others in the future and how we want to futureproof the
code.  We can say "already covers it" if we know the implementation
(especially, the code calls free() when it replaces opts->gpg_sign
always, so the other side you are choosing not to test will die the
same way) and assume it won't be broken (i.e. the attitude is OK for
whitebox testing).

I'd think that this particular case it is sufficient to test just
one; I doubt that adding another will increse the test load
measurably, though.

Re: [PATCH v2 1/5] core.aheadbehind: add new config setting

[Junio C Hamano]

Jonathan Nieder  writes:

>> Created core.aheadbehind config setting and core_ahead_behind
>> global variable.  This value defaults to true.
>>
>> This value will be used in the next few commits as the default value
>> for the --ahead-behind parameter.
>>
>> Signed-off-by: Jeff Hostetler 
>> ---
>>  Documentation/config.txt | 8 
>>  cache.h  | 1 +
>>  config.c | 5 +
>>  environment.c| 1 +
>>  4 files changed, 15 insertions(+)
>
> Not a reason to reroll on its own, but this seems out of order: the
> series is easier to explain and easier to merge down in stages if the
> patch for --ahead-behind comes first, then the config setting.
>
> More generally, new commandline flags tend to be less controversial
> than new config settings since they cannot affect a script by mistake,
> and for that reason, they can go earlier in the series.
>
> As a bonus, that makes it possible to include tests.  It's probably
> worth adding a test or two for this new config setting.
>
> [...]
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 9593bfa..c78d6be 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -895,6 +895,14 @@ core.abbrev::
>>  abbreviated object names to stay unique for some time.
>>  The minimum length is 4.
>>  
>> +core.aheadbehind::
>> +If true, tells commands like status and branch to print ahead and
>> +behind counts for the branch relative to its upstream branch.
>> +This computation may be very expensive when there is a great
>> +distance between the two branches.  If false, these commands
>> +only print that the two branches refer to different commits.
>> +Defaults to true.
>
> This doesn't seem like a particularly core feature to me.  Should it be
> e.g. status.aheadbehind (even though it also affects "git branch") or
> even something like diff.aheadbehind?  I'm not sure.

FWIW, I do not think it is core at all, either; sorry for not
anticipating that a wrong name will be picked without a proper
guidance when I saw the "not limited to status" mentioned in the
discussion, but I was sick and offline for a few days, so...

> I also wonder if there's a way to achieve the same benefit without
> having it be configurable.  E.g. if a branch is way behind, couldn't
> we terminate the walk early to get the same bounded cost per branch
> without requiring configuration?

Hmm, that is an interesting thought.

Re: [PATCH] t/helper/test-lazy-name-hash: fix compilation

[Stefan Beller]

> Heh; I do not think there particularly is much difference between
> stricter flags and DEVELOPER flags, but I would rather not lose the
> removal of duplicated 'today' I did while I queued the previous one
> ;-)


I did not realize it was queued anywhere, please ignore then.

Re: [PATCH] t/helper/test-lazy-name-hash: fix compilation

[Junio C Hamano]

Stefan Beller  writes:

> I was compiling origin/master today with the DEVELOPER compiler flags
> today and was greeted by
>
> t/helper/test-lazy-init-name-hash.c: In function ‘cmd_main’:
> t/helper/test-lazy-init-name-hash.c:172:5: error: ‘nr_threads_used’ may be 
> used uninitialized in this function [-Werror=maybe-uninitialized]
>  printf("avg [size %8d] [single %f] %c [multi %f %d]\n",
>  ^~~
>  nr,
>  ~~~
>  (double)avg_single/10,
>  ~~
>  (avg_single < avg_multi ? '<' : '>'),
>  ~
>  (double)avg_multi/10,
>  ~
>  nr_threads_used);
>  
> t/helper/test-lazy-init-name-hash.c:115:6: note: ‘nr_threads_used’ was 
> declared here
>   int nr_threads_used;
>   ^~~
>
> Fix this issue by assigning 0 to 'nr_threads_used'.
>
> Acked-by: Jonathan Tan 
> Signed-off-by: Jeff Hostetler 
> Signed-off-by: Stefan Beller 
> ---
>
> Slightly reworded the commit message. I'd really like this patch to be 
> included
> such that I can compile git with the DEVELOPER_CFLAGS flags.

Heh; I do not think there particularly is much difference between
stricter flags and DEVELOPER flags, but I would rather not lose the
removal of duplicated 'today' I did while I queued the previous one
;-)

>
>  t/helper/test-lazy-init-name-hash.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/helper/test-lazy-init-name-hash.c 
> b/t/helper/test-lazy-init-name-hash.c
> index 6368a89345..297fb01d61 100644
> --- a/t/helper/test-lazy-init-name-hash.c
> +++ b/t/helper/test-lazy-init-name-hash.c
> @@ -112,7 +112,7 @@ static void analyze_run(void)
>  {
>   uint64_t t1s, t1m, t2s, t2m;
>   int cache_nr_limit;
> - int nr_threads_used;
> + int nr_threads_used = 0;
>   int i;
>   int nr;

Re: [PATCH] send-pack: use internal argv_array of struct child_process

[Stefan Beller]

On Fri, Dec 22, 2017 at 12:14 AM, René Scharfe  wrote:
> Avoid a magic number of NULL placeholder values and a magic index by
> constructing the command line for pack-objects using the embedded
> argv_array of the child_process.  The resulting code is shorter and
> easier to extend.
>
> Signed-off-by: Rene Scharfe 

Reviewed-by: Stefan Beller 

> ---
>  send-pack.c | 28 +---
>  1 file changed, 9 insertions(+), 19 deletions(-)
>
> diff --git a/send-pack.c b/send-pack.c
> index a8cc6b266e..2112d3b27a 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -58,35 +58,25 @@ static int pack_objects(int fd, struct ref *refs, struct 
> oid_array *extra, struc
>  * the revision parameters to it via its stdin and
>  * let its stdout go back to the other end.
>  */
> -   const char *argv[] = {
> -   "pack-objects",
> -   "--all-progress-implied",
> -   "--revs",
> -   "--stdout",
> -   NULL,
> -   NULL,
> -   NULL,
> -   NULL,
> -   NULL,
> -   NULL,
> -   };
> struct child_process po = CHILD_PROCESS_INIT;
> FILE *po_in;
> int i;
> int rc;
>
> -   i = 4;
> +   argv_array_push(, "pack-objects");
> +   argv_array_push(, "--all-progress-implied");
> +   argv_array_push(, "--revs");
> +   argv_array_push(, "--stdout");

(useless nit of the day, no need to resend:)
These four statements could be done as pushl(a,b,c, NULL);
but it would not differ in readability, so I guess either is fine.

Thanks,
Stefan

> if (args->use_thin_pack)
> -   argv[i++] = "--thin";
> +   argv_array_push(, "--thin");
> if (args->use_ofs_delta)
> -   argv[i++] = "--delta-base-offset";
> +   argv_array_push(, "--delta-base-offset");
> if (args->quiet || !args->progress)
> -   argv[i++] = "-q";
> +   argv_array_push(, "-q");
> if (args->progress)
> -   argv[i++] = "--progress";
> +   argv_array_push(, "--progress");
> if (is_repository_shallow())
> -   argv[i++] = "--shallow";
> -   po.argv = argv;
> +   argv_array_push(, "--shallow");
> po.in = -1;
> po.out = args->stateless_rpc ? -1 : fd;
> po.git_cmd = 1;
> --
> 2.15.1

[PATCH] Fix urlencode format string on signed char.

[Julien Dusser]

Git credential fails with special char in password.
remote: Invalid username or password.
fatal: Authentication failed for

File ~/.git-credential contains badly urlencoded characters
%ffXX%ffYY instead of %XX%YY.

Add a cast to an unsigned char to fix urlencode use of %02x
on a char.

Signed-off-by: Julien Dusser 
---
 strbuf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/strbuf.c b/strbuf.c
index 323c49ceb..4d5a9ce55 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -658,7 +658,7 @@ static void strbuf_add_urlencode(struct strbuf *sb, const 
char *s, size_t len,
(!reserved && is_rfc3986_reserved(ch)))
strbuf_addch(sb, ch);
else
-   strbuf_addf(sb, "%%%02x", ch);
+   strbuf_addf(sb, "%%%02x", (unsigned char)ch);
}
 }
 
-- 
2.15.1

Re: [PATCH] sequencer: assign only free()able strings to gpg_sign

[Johannes Schindelin]

Hi Phillip,

On Fri, 22 Dec 2017, phillip.w...@talktalk.net wrote:

> I thought the bug could be triggered when commit.gpgsign was true and 
> it was not overriden on the commandline, is it worth adding a test for 
> that?

Everybody working with extensive test suites seems to write/blog these
days that you have to be careful to test meaningfully, i.e. you need to
avoid making running the test suite so expensive that developers start to
avoid running it.

In that light, what do we want to test? If we want to verify that the
gpg_sign is correctly allocated before it is free()d, then the test case I
added *already* covers it, and another test case would only increase the
runtime of the test suite (which, as I hinted above, I deem a bad thing).

So I'm really in favor of keeping the tests concise.

Ciao,
Dscho

Re: [PATCH] oidmap.h: strongly discourage using OIDMAP_INIT directly

[Brandon Williams]

On 12/22, Johannes Schindelin wrote:
> In Git's source code, we have this convention that quite a few data
> structures can be initialized using a macro *_INIT while defining an
> instance (instead of having to call a function to initialize the data
> structure). You will see that idiom quite a bit, e.g. `struct strbuf buf
> = STRBUF_INIT;`
> 
> This works for oidsets, too: `struct oidset oids = OIDSET_INIT;` is
> perfectly legal and you can use that data structure right away, without
> having to call `oidset_init()` first.
> 
> That pattern is violated by OIDMAP_INIT, though. The first call to
> oidmap_put() or oidmap_get() will succeed, but by mistake rather than by
> design: The underlying hashmap is not initialized correctly, as the
> cmpfn function pointer still points to NULL, but since there are no
> entries to be compared, cmpfn will not be called. Things break down,
> though, as soon as there is even one entry.
> 
> Rather than causing confusion, frustration and needless loss of time due
> to pointless debugging, let's *rename* OIDMAP_INIT so that developers
> who have gotten used to the pattern `struct xyz a = XYZ_INIT;` won't do
> that with oidmaps.
> 
> An alternative would be to introduce a HASHMAP_INIT(cmp_fn) and use that
> in oidmap.h. However, there are *no* call sites in Git's source code
> that would benefit from that macro, i.e. this would be premature
> optimization (and cost a lot more time than this here trivial patch).
> 
> Signed-off-by: Johannes Schindelin 
> ---
> Published-As: 
> https://github.com/dscho/git/releases/tag/discourage-OIDMAP_INIT-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git discourage-OIDMAP_INIT-v1
>  oidmap.h | 6 +-
>  oidset.h | 7 ++-
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/oidmap.h b/oidmap.h
> index 18f54cde143..1a73c392b79 100644
> --- a/oidmap.h
> +++ b/oidmap.h
> @@ -21,7 +21,11 @@ struct oidmap {
>   struct hashmap map;
>  };
>  
> -#define OIDMAP_INIT { { NULL } }
> +/*
> + * This macro initializes the data structure only incompletely, just enough
> + * to call oidmap_get() on an empty map. Use oidmap_init() instead.
> + */
> +#define OIDMAP_INIT_INCOMPLETELY { { NULL } }

This is one way of approaching the problem.  Couldn't we also take the
approach like we have with oidset and ensure that when oidmap_get() or
_put() are called that if the oidmap isn't initialized, we simply do
that then?

>  
>  /*
>   * Initializes an oidmap structure.
> diff --git a/oidset.h b/oidset.h
> index f4c9e0f9c04..a11d88edc1d 100644
> --- a/oidset.h
> +++ b/oidset.h
> @@ -22,7 +22,12 @@ struct oidset {
>   struct oidmap map;
>  };
>  
> -#define OIDSET_INIT { OIDMAP_INIT }
> +/*
> + * It is okay to initialize the map incompletely here because oidset_insert()
> + * will call oidset_init() (which will call oidmap_init()), and
> + * oidset_contains() works as intended even before oidset_init() was called.
> + */
> +#define OIDSET_INIT { OIDMAP_INIT_INCOMPLETELY }
>  
>  /**
>   * Returns true iff `set` contains `oid`.
> 
> base-commit: 936d1b989416a95f593bf81ccae8ac62cd83f279
> -- 
> 2.15.1.windows.2

-- 
Brandon Williams

[PATCH v2 1/2] commit doc: document that -c, -C, -F and --fixup with -m error

[Ævar Arnfjörð Bjarmason]

Document that providing any of -c, -C, -F and --fixup along with -m
will result in an error. Some variant of this has been errored about
explicitly since 0c091296c0 ("git-commit: log parameter updates.",
2005-08-08), but the documentation was never updated to reflect this.

Wording-by: Eric Sunshine 
Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/git-commit.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 8c74a2ca03..3fbb7352bc 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -144,6 +144,9 @@ OPTIONS
Use the given  as the commit message.
If multiple `-m` options are given, their values are
concatenated as separate paragraphs.
++
+The `-m` option is mutually exclusive with `-c`, `-C`, `-F`, and
+`--fixup`.
 
 -t ::
 --template=::
-- 
2.15.1.424.g9478a66081

[PATCH v2 0/2] support -m"" combined with commit --fixup

[Ævar Arnfjörð Bjarmason]

Here's a hopefully ready to apply v2 incorporating feedback from Eric
(thanks!). A tbdiff with v1 follows below.

Ævar Arnfjörð Bjarmason (2):
  commit doc: document that -c, -C, -F and --fixup with -m error
  commit: add support for --fixup  -m""

 Documentation/git-commit.txt | 2 ++
 builtin/commit.c | 8 +---
 t/t7500-commit.sh| 9 -
 3 files changed, 15 insertions(+), 4 deletions(-)

1: 7d5e2531ee ! 1: 82333992ec commit doc: document that -c, -C, -F and --fixup 
with -m error
@@ -7,6 +7,7 @@
 explicitly since 0c091296c0 ("git-commit: log parameter updates.",
 2005-08-08), but the documentation was never updated to reflect this.
 
+Wording-by: Eric Sunshine 
 Signed-off-by: Ævar Arnfjörð Bjarmason 
 
 diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
@@ -17,8 +18,8 @@
If multiple `-m` options are given, their values are
concatenated as separate paragraphs.
 ++
-+Combining the `-m` option and any of `-c`, `-C`, `-F` or `--fixup`
-+will result in an error.
++The `-m` option is mutually exclusive with `-c`, `-C`, `-F`, and
++`--fixup`.
  
  -t ::
  --template=::
2: bd78a211ed ! 2: 780de6e042 commit: add support for --fixup  
-m""
@@ -22,6 +22,21 @@
In such a case you might want to leave a small message,
e.g. "forgot this part, which broke XYZ".
 
+With this, --fixup  -m"More" -m"Details" will result in a
+commit message like:
+
+!fixup >
+
+More
+
+Details
+
+The reason the test being added here seems to squash "More" at the end
+of the subject line of the commit being fixed up is because the test
+code is using "%s%b" so the body immediately follows the subject, it's
+not a bug in this code, and other tests t7500-commit.sh do the same
+thing.
+
 When the --fixup option was initially added the "Option -m cannot be
 combined" error was expanded from -c, -C and -F to also include
 --fixup[1]
@@ -34,6 +49,7 @@
 1. d71b8ba7c9 ("commit: --fixup option for use with rebase
--autosquash", 2010-11-02)
 
+Helped-by: Eric Sunshine 
 Signed-off-by: Ævar Arnfjörð Bjarmason 
 
 diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
@@ -43,10 +59,9 @@
If multiple `-m` options are given, their values are
concatenated as separate paragraphs.
  +
--Combining the `-m` option and any of `-c`, `-C`, `-F` or `--fixup`
--will result in an error.
-+Combining the `-m` option and any of `-c`, `-C` or `-F` will result in
-+an error.
+-The `-m` option is mutually exclusive with `-c`, `-C`, `-F`, and
+-`--fixup`.
++The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`.
  
  -t ::
  --template=::

-- 
2.15.1.424.g9478a66081

Re: Usability outrage as far as I am concerned

[Cristian Achim]

> Can you show the output of "git remote"

# in usb_subfolder
$git remote
origin
$

#in home_subfolder
$git remote
$

> and also
> clearly explain with details the layout of what the folders are and
> what is or is not a repository?

Take the following update into consideration and then reread my first
email hopefully with improved clarity:

'home_subfolder' is the path on disk inside my user account home
folder in the 'home' root folder to the initial repo from which I
meant to do a backup.

'usb_subfolder' is the path on disk in the 'media' root folder to the
initial empty folder into which I wanted to do the backup above that
points into a usb stick I mounted in the default Kubuntu KDE file
manager way of mounting usb stick folder hierarchies.

Current situation is that 'git log' in both home_subfolder and
usb_subfolder show the same hash with only one branch in both. From
usb_subfolder 'git pull home_subfolder' is broken as in the original
message.

[PATCH] status: add a failing test showing a core.untrackedCache bug

[Ævar Arnfjörð Bjarmason]

The untracked cache gets confused when a directory is swapped out for
a symlink to another directory. Whatever files are inside the target
of the symlink will be incorrectly shown as untracked. This issue does
not happen if the symlink links to another file, only if it links to
another directory.

A stand-alone testcase for copying into a terminal:

(
rm -rf /tmp/testrepo &&
git init /tmp/testrepo &&
cd /tmp/testrepo &&
mkdir x y &&
touch x/a y/b &&
git add x/a y/b &&
git commit -msnap &&
git rm -rf y &&
ln -s x y &&
git add y &&
git commit -msnap2 &&
git checkout HEAD~ &&
git status &&
git checkout master &&
sleep 1 &&
git status &&
git status
)

This will incorrectly show y/a as an untracked file. Both the "git
status" call right before "git checkout master" and the "sleep 1"
after the "checkout master" are needed to reproduce this, presumably
due to the untracked cache tracking on the basis of cached whole
seconds from stat(2).

When git gets into this state, a workaround to fix it is to issue a
one-off:

git -c core.untrackedCache=false status

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/t7063-status-untracked-cache.sh | 45 +++
 1 file changed, 45 insertions(+)

diff --git a/t/t7063-status-untracked-cache.sh 
b/t/t7063-status-untracked-cache.sh
index e5fb892f95..7cf1e2c091 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -22,6 +22,12 @@ avoid_racy() {
sleep 1
 }
 
+status_is_clean() {
+   >../status.expect &&
+   git status --porcelain >../status.actual &&
+   test_cmp ../status.expect ../status.actual
+}
+
 test_lazy_prereq UNTRACKED_CACHE '
{ git update-index --test-untracked-cache; ret=$?; } &&
test $ret -ne 1
@@ -683,4 +689,43 @@ test_expect_success 'untracked cache survives a commit' '
test_cmp ../before ../after
 '
 
+test_expect_success 'teardown worktree' '
+cd ..
+'
+
+test_expect_success 'setup worktree for symlink test' '
+   git init worktree-symlink &&
+   cd worktree-symlink &&
+   git config core.untrackedCache true &&
+   mkdir one two &&
+   touch one/file two/file &&
+   git add one/file two/file &&
+   git commit -m"first commit" &&
+   git rm -rf one &&
+   ln -s two one &&
+   git add one &&
+   git commit -m"second commit"
+'
+
+test_expect_failure '"status" after symlink replacement should be clean with 
UC=true' '
+   git checkout HEAD~ &&
+   status_is_clean &&
+   status_is_clean &&
+   git checkout master &&
+   avoid_racy &&
+   status_is_clean &&
+   status_is_clean
+'
+
+test_expect_success '"status" after symlink replacement should be clean with 
UC=false' '
+   git config core.untrackedCache false &&
+   git checkout HEAD~ &&
+   status_is_clean &&
+   status_is_clean &&
+   git checkout master &&
+   avoid_racy &&
+   status_is_clean &&
+   status_is_clean
+'
+
 test_done
-- 
2.15.1.424.g9478a66081

Re: [PATCH] sequencer: assign only free()able strings to gpg_sign

[phillip.w...@talktalk.net]

>Original Message
>From: johannes.schinde...@gmx.de
>Date: 22/12/2017 11:50 
>To: 
>Cc: "Junio C Hamano", "Phillip Wood", "Kaartic Sivaraam"
>Subj: [PATCH] sequencer: assign only free()able strings to gpg_sign
>
>The gpg_sign member of the replay_opts structure is of type `char *`,
>meaning that the sequencer deems the string to which gpg_sign points 
to
>be under its custody, i.e. it needs to be free()d by the sequencer.
>
>Therefore, let's only assign malloc()ed buffers to it.
>
>Reported-by: Kaartic Sivaraam 
>Signed-off-by: Johannes Schindelin 
>---
>
>   Phillip, if you want to squash these changes into your patches,
>   I'd totally fine with that.
>

Hi Johannes, thanks for putting this together, the patch it fixes is 
already in next so I think it'd be best to leave this one separate. I 
wonder if it would be worth adding another test, see below.

Best Wishes


Phillip

>Based-On: pw/sequencer-in-process-commit at https://github.com/dscho/git

>Fetch-Base-Via: git fetch https://github.com/dscho/git pw/sequencer-
in-process-commit
>Published-As: 
>https://github.com/dscho/git/releases/tag/sequencer-owns-gpg-sign-v1

>Fetch-It-Via: git fetch https://github.com/dscho/git sequencer-owns-
gpg-sign-v1
> sequencer.c   |  2 +-
> t/t3404-rebase-interactive.sh | 10 ++
> 2 files changed, 11 insertions(+), 1 deletion(-)
>
>diff --git a/sequencer.c b/sequencer.c
>index 7051b20b762..1b2599668f5 100644
>--- a/sequencer.c
>+++ b/sequencer.c
>@@ -160,7 +160,7 @@ static int git_sequencer_config(const char *k, 
const char *v, void *cb)
>   }
> 
>   if (!strcmp(k, "commit.gpgsign")) {
>-  opts->gpg_sign = git_config_bool(k, v) ? "" : NULL;
>+  opts->gpg_sign = git_config_bool(k, v) ? xstrdup("") : NULL;
>   return 0;
>   }
> 
>diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-
interactive.sh
>index 9ed0a244e6c..040ef1a4dbc 100755
>--- a/t/t3404-rebase-interactive.sh
>+++ b/t/t3404-rebase-interactive.sh
>@@ -1318,6 +1318,16 @@ test_expect_success 'editor saves as CR/LF' '
> 
> SQ="'"
> test_expect_success 'rebase -i --gpg-sign=' '
>+  test_when_finished "test_might_fail git rebase --abort" &&
>+  set_fake_editor &&
>+  FAKE_LINES="edit 1" git rebase -i --gpg-sign="\"S I Gner\"" HEAD^ \
>+  >out 2>err &&
>+  test_i18ngrep "$SQ-S\"S I Gner\"$SQ" err
>+'
>+
>+test_expect_success 'rebase -i --gpg-sign= overrides commit.
gpgSign' '
>+  test_when_finished "test_might_fail git rebase --abort" &&
>+  test_config commit.gpgsign true &&
>   set_fake_editor &&
>   FAKE_LINES="edit 1" git rebase -i --gpg-sign="\"S I Gner\"" HEAD^ \
>   >out 2>err &&


I thought the bug could be triggered when commit.gpgsign was true and 
it was not overriden on the commandline, is it worth adding a test for 
that?


>base-commit: 28d6daed4f119940ace31e523b3b272d3d153d04
>-- 
>2.15.1.windows.2
>

[PATCH v2] http: use internal argv_array of struct child_process

[René Scharfe]

Avoid a strangely magic array size (it's slightly too big) and explicit
index numbers by building the command line for index-pack using the
embedded argv_array of the child_process.  Add the flag -o and its
argument with argv_array_pushl() to make it obvious that they belong
together.  The resulting code is shorter and easier to extend.

Helped-by: Eric Sunshine 
Signed-off-by: Rene Scharfe 
---
Change from v1: use argv_array_pushl for -o and tmp_idx.

Thanks, Eric, good idea!

 http.c | 11 +++
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/http.c b/http.c
index 215bebef1b..9f2fcc9ec4 100644
--- a/http.c
+++ b/http.c
@@ -2025,7 +2025,6 @@ int finish_http_pack_request(struct http_pack_request 
*preq)
char *tmp_idx;
size_t len;
struct child_process ip = CHILD_PROCESS_INIT;
-   const char *ip_argv[8];
 
close_pack_index(p);
 
@@ -2041,13 +2040,9 @@ int finish_http_pack_request(struct http_pack_request 
*preq)
die("BUG: pack tmpfile does not end in .pack.temp?");
tmp_idx = xstrfmt("%.*s.idx.temp", (int)len, preq->tmpfile);
 
-   ip_argv[0] = "index-pack";
-   ip_argv[1] = "-o";
-   ip_argv[2] = tmp_idx;
-   ip_argv[3] = preq->tmpfile;
-   ip_argv[4] = NULL;
-
-   ip.argv = ip_argv;
+   argv_array_push(, "index-pack");
+   argv_array_pushl(, "-o", tmp_idx, NULL);
+   argv_array_push(, preq->tmpfile);
ip.git_cmd = 1;
ip.no_stdin = 1;
ip.no_stdout = 1;
-- 
2.15.1

[PATCH] sequencer: assign only free()able strings to gpg_sign

[Johannes Schindelin]

The gpg_sign member of the replay_opts structure is of type `char *`,
meaning that the sequencer deems the string to which gpg_sign points to
be under its custody, i.e. it needs to be free()d by the sequencer.

Therefore, let's only assign malloc()ed buffers to it.

Reported-by: Kaartic Sivaraam 
Signed-off-by: Johannes Schindelin 
---

Phillip, if you want to squash these changes into your patches,
I'd totally fine with that.

Based-On: pw/sequencer-in-process-commit at https://github.com/dscho/git
Fetch-Base-Via: git fetch https://github.com/dscho/git 
pw/sequencer-in-process-commit
Published-As: 
https://github.com/dscho/git/releases/tag/sequencer-owns-gpg-sign-v1
Fetch-It-Via: git fetch https://github.com/dscho/git sequencer-owns-gpg-sign-v1
 sequencer.c   |  2 +-
 t/t3404-rebase-interactive.sh | 10 ++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 7051b20b762..1b2599668f5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -160,7 +160,7 @@ static int git_sequencer_config(const char *k, const char 
*v, void *cb)
}
 
if (!strcmp(k, "commit.gpgsign")) {
-   opts->gpg_sign = git_config_bool(k, v) ? "" : NULL;
+   opts->gpg_sign = git_config_bool(k, v) ? xstrdup("") : NULL;
return 0;
}
 
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 9ed0a244e6c..040ef1a4dbc 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1318,6 +1318,16 @@ test_expect_success 'editor saves as CR/LF' '
 
 SQ="'"
 test_expect_success 'rebase -i --gpg-sign=' '
+   test_when_finished "test_might_fail git rebase --abort" &&
+   set_fake_editor &&
+   FAKE_LINES="edit 1" git rebase -i --gpg-sign="\"S I Gner\"" HEAD^ \
+   >out 2>err &&
+   test_i18ngrep "$SQ-S\"S I Gner\"$SQ" err
+'
+
+test_expect_success 'rebase -i --gpg-sign= overrides commit.gpgSign' '
+   test_when_finished "test_might_fail git rebase --abort" &&
+   test_config commit.gpgsign true &&
set_fake_editor &&
FAKE_LINES="edit 1" git rebase -i --gpg-sign="\"S I Gner\"" HEAD^ \
>out 2>err &&

base-commit: 28d6daed4f119940ace31e523b3b272d3d153d04
-- 
2.15.1.windows.2

Re: Error in `git': free(): invalid pointer (was Re: [PATCH] sequencer: improve config handling)

[Johannes Schindelin]

Hi Kaartic,

On Thu, 21 Dec 2017, Kaartic Sivaraam wrote:

> Speaking of trace, (thanks to Dscho!) the one I got using 'valgrind'
> (with `--leak-check=full` option) can be found below. I've kept only
> relevant parts of it to avoid unwanted noise of `set -x`. Let me know
> if that's needed too.
> 
> ...
> 
> + git diff-files --quiet --ignore-submodules
> + git diff-index --cached --quiet --ignore-submodules HEAD --
> + test 0 = 1
> + test -z 1
> + valgrind --leak-check=full git rebase--helper --continue
> ==10384== Memcheck, a memory error detector
> ==10384== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
> ==10384== Using Valgrind-3.12.0.SVN and LibVEX; rerun with -h for copyright 
> info
> ==10384== Command: git rebase--helper --continue
> ==10384== 
> ==10384== Invalid free() / delete / delete[] / realloc()
> ==10384==at 0x4C2CDDB: free (in 
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==10384==by 0x24E3F8: read_populate_opts (sequencer.c:1964)
> ==10384==by 0x24E3F8: sequencer_continue (sequencer.c:2753)
> ==10384==by 0x1735DC: cmd_rebase__helper (rebase--helper.c:52)
> ==10384==by 0x11B847: run_builtin (git.c:346)
> ==10384==by 0x11B847: handle_builtin (git.c:554)
> ==10384==by 0x11BB05: run_argv (git.c:606)
> ==10384==by 0x11BB05: cmd_main (git.c:683)
> ==10384==by 0x11AC0B: main (common-main.c:43)
> ==10384==  Address 0x2a898a is in a r-x mapped file 
> /mnt/Source/Git/git-next/git segment
> ==10384== 
> Successfully rebased and updated refs/heads/public.
> ==10384== 
> ==10384== HEAP SUMMARY:
> ==10384== in use at exit: 680,882 bytes in 332 blocks
> ==10384==   total heap usage: 717 allocs, 386 frees, 1,709,293 bytes allocated
> ==10384== 
> ==10384== 2,053 (2,048 direct, 5 indirect) bytes in 1 blocks are definitely 
> lost in loss record 75 of 83
> ==10384==at 0x4C2BADF: malloc (in 
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==10384==by 0x4C2DE5F: realloc (in 
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==10384==by 0x27E0FE: xrealloc (wrapper.c:138)
> ==10384==by 0x2072A3: add_object_array_with_path (object.c:319)
> ==10384==by 0x23D745: add_pending_object_with_path (revision.c:163)
> ==10384==by 0x24030E: add_pending_object_with_mode (revision.c:170)
> ==10384==by 0x24030E: add_pending_object (revision.c:176)
> ==10384==by 0x24030E: add_head_to_pending (revision.c:188)
> ==10384==by 0x280EFA: has_uncommitted_changes.part.17 (wt-status.c:2288)
> ==10384==by 0x24DF88: commit_staged_changes (sequencer.c:2705)
> ==10384==by 0x24DF88: sequencer_continue (sequencer.c:2749)
> ==10384==by 0x1735DC: cmd_rebase__helper (rebase--helper.c:52)
> ==10384==by 0x11B847: run_builtin (git.c:346)
> ==10384==by 0x11B847: handle_builtin (git.c:554)
> ==10384==by 0x11BB05: run_argv (git.c:606)
> ==10384==by 0x11BB05: cmd_main (git.c:683)
> ==10384==by 0x11AC0B: main (common-main.c:43)
> ==10384== 
> ==10384== LEAK SUMMARY:
> ==10384==definitely lost: 2,048 bytes in 1 blocks
> ==10384==indirectly lost: 5 bytes in 1 blocks
> ==10384==  possibly lost: 0 bytes in 0 blocks
> ==10384==still reachable: 678,829 bytes in 330 blocks
> ==10384== suppressed: 0 bytes in 0 blocks
> ==10384== Reachable blocks (those to which a pointer was found) are not shown.
> ==10384== To see them, rerun with: --leak-check=full --show-leak-kinds=all
> ==10384== 
> ==10384== For counts of detected and suppressed errors, rerun with: -v
> ==10384== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)
> + exit
> 
> 
> I think I didn't mention I've set `commit.gpgsign` to `true` for that
> repo, did I?

Hah! I had troubles to associate the correct line in my versions of Git's
source code (the line numbers alone are only reliable if you also have a
commit from which the Git binaries were built), but I did this free() at
(https://github.com/git/git/blob/cd54ea2b18/sequencer.c#L1975:

if (read_oneliner(, rebase_path_gpg_sign_opt(), 1)) {
if (!starts_with(buf.buf, "-S"))
strbuf_reset();
else {
free(opts->gpg_sign);
^
opts->gpg_sign = xstrdup(buf.buf + 2);
}
strbuf_reset();
}

The culprit is that we have these really unclear ownership rules, where it
is not at all clear who is responsible for releasing allocated memory:
caller or callee? (Hannes would not rightfully point out that this would
be a non-issue if we would switch to C++). In C, the common pattern is to
use `const char *` for users that are *not* supposed to take ownership,
and `char *` for users that are supposed to take custody. And in this
instance, `gpg_sign` should be owned by `struct replay_opts` because of
this (https://github.com/git/git/blob/cd54ea2b18/sequencer.h#L38):

char *gpg_sign;

[PATCH] oidmap.h: strongly discourage using OIDMAP_INIT directly

[Johannes Schindelin]

In Git's source code, we have this convention that quite a few data
structures can be initialized using a macro *_INIT while defining an
instance (instead of having to call a function to initialize the data
structure). You will see that idiom quite a bit, e.g. `struct strbuf buf
= STRBUF_INIT;`

This works for oidsets, too: `struct oidset oids = OIDSET_INIT;` is
perfectly legal and you can use that data structure right away, without
having to call `oidset_init()` first.

That pattern is violated by OIDMAP_INIT, though. The first call to
oidmap_put() or oidmap_get() will succeed, but by mistake rather than by
design: The underlying hashmap is not initialized correctly, as the
cmpfn function pointer still points to NULL, but since there are no
entries to be compared, cmpfn will not be called. Things break down,
though, as soon as there is even one entry.

Rather than causing confusion, frustration and needless loss of time due
to pointless debugging, let's *rename* OIDMAP_INIT so that developers
who have gotten used to the pattern `struct xyz a = XYZ_INIT;` won't do
that with oidmaps.

An alternative would be to introduce a HASHMAP_INIT(cmp_fn) and use that
in oidmap.h. However, there are *no* call sites in Git's source code
that would benefit from that macro, i.e. this would be premature
optimization (and cost a lot more time than this here trivial patch).

Signed-off-by: Johannes Schindelin 
---
Published-As: 
https://github.com/dscho/git/releases/tag/discourage-OIDMAP_INIT-v1
Fetch-It-Via: git fetch https://github.com/dscho/git discourage-OIDMAP_INIT-v1
 oidmap.h | 6 +-
 oidset.h | 7 ++-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/oidmap.h b/oidmap.h
index 18f54cde143..1a73c392b79 100644
--- a/oidmap.h
+++ b/oidmap.h
@@ -21,7 +21,11 @@ struct oidmap {
struct hashmap map;
 };
 
-#define OIDMAP_INIT { { NULL } }
+/*
+ * This macro initializes the data structure only incompletely, just enough
+ * to call oidmap_get() on an empty map. Use oidmap_init() instead.
+ */
+#define OIDMAP_INIT_INCOMPLETELY { { NULL } }
 
 /*
  * Initializes an oidmap structure.
diff --git a/oidset.h b/oidset.h
index f4c9e0f9c04..a11d88edc1d 100644
--- a/oidset.h
+++ b/oidset.h
@@ -22,7 +22,12 @@ struct oidset {
struct oidmap map;
 };
 
-#define OIDSET_INIT { OIDMAP_INIT }
+/*
+ * It is okay to initialize the map incompletely here because oidset_insert()
+ * will call oidset_init() (which will call oidmap_init()), and
+ * oidset_contains() works as intended even before oidset_init() was called.
+ */
+#define OIDSET_INIT { OIDMAP_INIT_INCOMPLETELY }
 
 /**
  * Returns true iff `set` contains `oid`.

base-commit: 936d1b989416a95f593bf81ccae8ac62cd83f279
-- 
2.15.1.windows.2

Re: Error in `git': free(): invalid pointer (was Re: [PATCH] sequencer: improve config handling)

[phillip.w...@talktalk.net]

>Original Message
>From: kaartic.sivar...@gmail.com
>Date: 21/12/2017 17:14 
>To: "phillip.w...@talktalk.net", "Phillip 
Wood", "Git Mailing List"
>Cc: "Johannes Schindelin"
>Subj: Re: Error in `git: free(): invalid pointer (was Re: [PATCH] 
sequencer: improve config handling)
>
>On Thu, 2017-12-21 at 16:53 +, phillip.w...@talktalk.net wrote:
>> Hm, There is a problem with sequencer_remove_state() which does 
>> 
>> free(opts->gpg_sign)
>> 
>> however unless a gpg key was given on the commandline, opts->gpg is 
>> initialized to "" which is statically allocated. 
>> 
>> This untested diff should fix that,
>
>It did seem to. I don't get that error any more.

That's good, thanks for testing it

>>  but I'm not sure if you're problem 
>> is related to it
>
>As the fix you suggested did work, I think the problem is related. Did
>you have anything else in mind when you said you're not sure whether 
or
>not it's related?

I didn't have anything else in mind, but at that point I hadn't noticed 
that one of your previous messages said you had gpg signing turned on - 
as you do it makes sense that the patch fixed it.


Thanks again for reporting this and testing the fix - it seems the test 
suite has a bit of a blind spot when it comes to gpg signing, I guess 
it's difficult to set up a key for tests

I'll send a proper patch when I have time in a few days

Best Wishes

Phillip

Re: [PATCH] http: use internal argv_array of struct child_process

[Eric Sunshine]

On Fri, Dec 22, 2017 at 3:14 AM, René Scharfe  wrote:
> Avoid a strangely magic array size (it's slightly too big) and explicit
> index numbers by building the command line for index-pack using the
> embedded argv_array of the child_process.  The resulting code is shorter
> and easier to extend.
>
> Signed-off-by: Rene Scharfe 
> ---
> diff --git a/http.c b/http.c
> @@ -2041,13 +2040,10 @@ int finish_http_pack_request(struct http_pack_request 
> *preq)
> -   ip_argv[0] = "index-pack";
> -   ip_argv[1] = "-o";
> -   ip_argv[2] = tmp_idx;
> -   ip_argv[3] = preq->tmpfile;
> -   ip_argv[4] = NULL;
> -
> -   ip.argv = ip_argv;
> +   argv_array_push(, "index-pack");
> +   argv_array_push(, "-o");
> +   argv_array_push(, tmp_idx);
> +   argv_array_push(, preq->tmpfile);

Not necessarily worth a re-roll, but using the "pushl" variant would
make it clear that "-o" and tmp_idx are related and would ensure that
they don't accidentally get split up if someone inserts a new "push"
in the sequence in the future.

argv_array_push(, "index-pack");
argv_array_pushl(, "-o", tmp_idx, NULL);
argv_array_push(, preq->tmpfile);

Assalamu`Alaikum.

[Mohammad Ouattara]

Greetings from Dr. mohammad ouattara.
 
Assalamu`Alaikum.
 
My Name is Dr. mohammad ouattara, I am a banker by profession. I'm from 
Ouagadougou, Burkina Faso, West Africa. My reason for contacting you is to 
transfer an abandoned $14.6M to your account.
 
The owner of this fund died since 2004 with his Next Of Kin. I want to present 
you to the bank as the Next of Kin/beneficiary of this fund.
 
Further details of the transaction shall be forwarded to you as soon as I 
receive your return mail indicating your interest.

1) YOUR FULL NAME...
(2) YOUR AGE AND SEX
(3) YOUR CONTACT ADDRESS..
(4) YOUR PRIVATE PHONE N0..
(5) FAX NUMBER..
(6) YOUR COUNTRY OF ORIGIN..
(7) YOUR OCCUPATION.

Have a great day,
Dr. mohammad ouattara.
 
 

 

[PATCH] send-pack: use internal argv_array of struct child_process

[René Scharfe]

Avoid a magic number of NULL placeholder values and a magic index by
constructing the command line for pack-objects using the embedded
argv_array of the child_process.  The resulting code is shorter and
easier to extend.

Signed-off-by: Rene Scharfe 
---
 send-pack.c | 28 +---
 1 file changed, 9 insertions(+), 19 deletions(-)

diff --git a/send-pack.c b/send-pack.c
index a8cc6b266e..2112d3b27a 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -58,35 +58,25 @@ static int pack_objects(int fd, struct ref *refs, struct 
oid_array *extra, struc
 * the revision parameters to it via its stdin and
 * let its stdout go back to the other end.
 */
-   const char *argv[] = {
-   "pack-objects",
-   "--all-progress-implied",
-   "--revs",
-   "--stdout",
-   NULL,
-   NULL,
-   NULL,
-   NULL,
-   NULL,
-   NULL,
-   };
struct child_process po = CHILD_PROCESS_INIT;
FILE *po_in;
int i;
int rc;
 
-   i = 4;
+   argv_array_push(, "pack-objects");
+   argv_array_push(, "--all-progress-implied");
+   argv_array_push(, "--revs");
+   argv_array_push(, "--stdout");
if (args->use_thin_pack)
-   argv[i++] = "--thin";
+   argv_array_push(, "--thin");
if (args->use_ofs_delta)
-   argv[i++] = "--delta-base-offset";
+   argv_array_push(, "--delta-base-offset");
if (args->quiet || !args->progress)
-   argv[i++] = "-q";
+   argv_array_push(, "-q");
if (args->progress)
-   argv[i++] = "--progress";
+   argv_array_push(, "--progress");
if (is_repository_shallow())
-   argv[i++] = "--shallow";
-   po.argv = argv;
+   argv_array_push(, "--shallow");
po.in = -1;
po.out = args->stateless_rpc ? -1 : fd;
po.git_cmd = 1;
-- 
2.15.1

[PATCH] http: use internal argv_array of struct child_process

[René Scharfe]

Avoid a strangely magic array size (it's slightly too big) and explicit
index numbers by building the command line for index-pack using the
embedded argv_array of the child_process.  The resulting code is shorter
and easier to extend.

Signed-off-by: Rene Scharfe 
---
 http.c | 12 
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/http.c b/http.c
index 215bebef1b..b22b4ada58 100644
--- a/http.c
+++ b/http.c
@@ -2025,7 +2025,6 @@ int finish_http_pack_request(struct http_pack_request 
*preq)
char *tmp_idx;
size_t len;
struct child_process ip = CHILD_PROCESS_INIT;
-   const char *ip_argv[8];
 
close_pack_index(p);
 
@@ -2041,13 +2040,10 @@ int finish_http_pack_request(struct http_pack_request 
*preq)
die("BUG: pack tmpfile does not end in .pack.temp?");
tmp_idx = xstrfmt("%.*s.idx.temp", (int)len, preq->tmpfile);
 
-   ip_argv[0] = "index-pack";
-   ip_argv[1] = "-o";
-   ip_argv[2] = tmp_idx;
-   ip_argv[3] = preq->tmpfile;
-   ip_argv[4] = NULL;
-
-   ip.argv = ip_argv;
+   argv_array_push(, "index-pack");
+   argv_array_push(, "-o");
+   argv_array_push(, tmp_idx);
+   argv_array_push(, preq->tmpfile);
ip.git_cmd = 1;
ip.no_stdin = 1;
ip.no_stdout = 1;
-- 
2.15.1