Re: bitmap creation failed

[Jeff King]

On Thu, Sep 01, 2016 at 11:16:49AM -0700, gjarms wrote:

> Hi Git Experts,
> 
> We have been exploring various ways to improve git cloning time, one among
> them is using bitmap which is suppose to save time "counting objects".  but
> i have problem creating bitmap since the repository contains 100's of pack
> files. the bitmap file is not created when i use "git gc".
> 
> I have the following entries in my .gitconfig.
> 
> [pack]
> packSizeLimit = 10m
> writebitmaps = on
> writeBitmapHashCache = on
> 
> If i just dont use "packSizeLimit = 10m", then bitmap is created just by
> running git gc
> 
> Can you please make me understand ?, What i understood is that the bitmap is
> created when there is a single pack file, but if i split it into multiple
> pack file, the bitmap generation fails with the warning
> "warning: disabling bitmap writing, as some objects are not being packed".

That's weird. I'd expect it to say:

  disabling bitmap writing, packs are split due to pack.packSizeLimit

At least since v2.8.3, which has 9cea46cdda. Before that it wouldn't
have printed anything, and just silently turned off bitmaps.

The "some objects are not being packed" warning should only come when
want_object_in_pack() says we don't want an object. That's generally
because the object is either found in a shared alternates repository, or
is in a .keep pack. Though in the latter case, unless you've set
repack.packKeptObjects manually, we'll pack it anyway when bitmaps are
in effect (since ee34a2bead, in v2.0.0).

That confusion aside, you almost certainly should not be setting
packSizeLimit, and definitely not to something so low. Git will not
store cross-pack deltas, so you miss out on tons of delta opportunities.
As a result:

  1. Your on-disk repository size will balloon. So you'll have a hundred
 10m packs rather than one 200mb pack.

  2. Your clone times will also grow, as git will try to find new deltas
 between the objects in various packs independently for each clone.

-Peff

Re: Should "git symbolic-ref -d HEAD" be forbidden?

[Jeff King]

On Thu, Sep 01, 2016 at 03:31:28PM -0700, Junio C Hamano wrote:

> -- >8 --
> Subject: symbolic-ref -d: do not allow removal of HEAD
> 
> If you delete the symbolic-ref HEAD from a repository, Git no longer
> considers it valid, and even "git symbolic-ref HEAD refs/heads/master"
> would not be able to recover from that state.
> 
> In the spirit similar to afe5d3d5 ("symbolic ref: refuse non-ref
> targets in HEAD", 2009-01-29), forbid removal of HEAD.
> 
> Signed-off-by: Junio C Hamano 
> ---

Makes sense. You might want to change "it" in "no longer considers it
valid" to "the repository". At first I thought "it" referred to the
symref. Which obviously shouldn't be valid after being deleted. :)

>  I decided against it for now for no good reason, other than I am a
>  bit superstitious, but it may be a good idea to move these safety
>  checks to delete_ref() and create_symref() in the longer term.

Yeah, that somehow feels weird and too low-level to me. After all, we
_do_ want to drop HEAD as a symref when we turn it into a detached HEAD.
The point of this (and afe5d3d5) is to prevent people from shooting
themselves in the foot. Internal Git code should know to avoid this
foot-shooting itself.

OTOH, I think "git update-ref --no-deref -d HEAD" is another user-facing
hole-in-foot opportunity, and it would be blocked by putting this into
delete_ref().

> -test_expect_success 'symbolic-ref deletes HEAD' '
> - git symbolic-ref -d HEAD &&
> +test_expect_success 'HEAD cannot be removed' '
> + test_must_fail git symbolic-ref -d HEAD
> +'
> +
> +test_expect_success 'symbolic-ref can be deleted' '
> + git symbolic-ref NOTHEAD refs/heads/foo &&
> + git symbolic-ref -d NOTHEAD &&
>   test_path_is_file .git/refs/heads/foo &&
> - test_path_is_missing .git/HEAD
> + test_path_is_missing .git/NOTHEAD
>  '
>  reset_to_sane

Do you want another "reset_to_sane" call after your new test? Otherwise
if it fails the "symbolic-ref can be deleted" test will start operating
on the parent repository.

-Peff

Re: bitmap creation failed

[Stefan Beller]

On Thu, Sep 1, 2016 at 11:16 AM, gjarms  wrote:
> Hi Git Experts,
>
> We have been exploring various ways to improve git cloning time, one among
> them is using bitmap which is suppose to save time "counting objects".  but
> i have problem creating bitmap since the repository contains 100's of pack
> files. the bitmap file is not created when i use "git gc".
>
> I have the following entries in my .gitconfig.
>
> [pack]
> packSizeLimit = 10m
> writebitmaps = on
> writeBitmapHashCache = on
>
> If i just dont use "packSizeLimit = 10m", then bitmap is created just by
> running git gc
>
> Can you please make me understand ?, What i understood is that the bitmap is
> created when there is a single pack file, but if i split it into multiple
> pack file, the bitmap generation fails with the warning
> "warning: disabling bitmap writing, as some objects are not being packed".

So I guess your single pack is larger than 10m, so it tries to create
multiple packs,
and that is not supported as bitmaps only operate on one pack.

Stefan

>
> Regards,
> Arumuga
>
>
>
> --
> View this message in context: 
> http://git.661346.n2.nabble.com/bitmap-creation-failed-tp7657450.html
> Sent from the git mailing list archive at Nabble.com.

Re: [PATCH 22/22] sequencer: refactor write_message()

[Jakub Narębski]

Hello Johannes,

W dniu 01.09.2016 o 16:20, Johannes Schindelin pisze:
> On Thu, 1 Sep 2016, Jakub Narębski wrote: 
>> W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:

>>> if (commit_lock_file(_file) < 0)
>>> return error(_("Error wrapping up %s."), filename);
>>
>> Another "while at it"... though the one that can be safely postponed
>> (well, the make message easier to understand part, not the quote
>> filename part):
>>
>>  return error(_("Error wrapping up writing to '%s'."), filename);
> 
> As I inherited this message, I'll keep it.

Well, please then add quotes while at it, at least, for consistency

return error(_("Error wrapping up '%s'."), filename);

Best,
-- 
Jakub Narębski

Re: [PATCH 21/22] sequencer: left-trim the lines read from the script

[Jakub Narębski]

Hello Johannes,

W dniu 01.09.2016 o 16:13, Johannes Schindelin pisze: 
> On Thu, 1 Sep 2016, Jakub Narębski wrote:
 
>> 'bol' is beginning-of-line, isn't it (a complement to eol)?
> 
> Yep. How did you guess? :-)

Wouldn't 'beg' and 'end' instead of 'bol' and 'eol' be easier
to understand, thus more readable?

-- 
Jakub Narębski

Re: [PATCH 19/22] sequencer: support cleaning up commit messages

[Jakub Narębski]

Hello Johannes,

W dniu 01.09.2016 o 15:56, Johannes Schindelin pisze: 
> On Thu, 1 Sep 2016, Jakub Narębski wrote:

>> It's a pity that emulation of named parameters in C requires
>> relying on designated inits from C99
>>
>>   typedef struct {
>> double pressure, moles, temp;
>>   } ideal_struct;
>>
>>   #define ideal_pressure(...) 
>> ideal_pressure_base((ideal_struct){.pressure=1,   \
>> .moles=1, .temp=273.15, __VA_ARGS__})
>>
>>   double ideal_pressure_base(ideal_struct in)
>>   {
>> return 8.314 * in.moles*in.temp/in.pressure;
>>   }
>>
>>   ... ideal_pressure(.moles=2, .temp=373.15) ...

Forgot to add citation:

[1] Ben Klemens "21st Century C: C Tips from the New School", 2nd Ed. (2014),
O'Reilly Media, chapter 10. "Better Structures", subsection
"Optional and Named Arguments"

> 
> Yeah, that looks unwieldy ;-)
>

Declaration needs some trickery, but use is much, much more readable
(if we cannot use sensibly named variables for passing arguments):

  ideal_pressure()
  ideal_pressure(.temp=373.15)
  ideal_pressure(.moles=2)
  ideal_pressure(.moles=2, .temp=373.15) 

It is even better if there are large amount of parameters:

  res = amortization(.amount=20, .inflation=3,
 .show_table=0, .extra_payoff=100)

vs

  double amortize(double amt, double rate, double inflation, int months,
int selloff_month, double extra_payoff, int verbose,
double *interest_pv, double *duration, double *monthly_payment);

 
But we can't use it in Git, anyway
-- 
Jakub Narębski

Re: [PATCH 16/22] sequencer: prepare for rebase -i's GPG settings

[Jakub Narębski]

Hello Johannes,

W dniu 01.09.2016 o 15:33, Johannes Schindelin pisze:
> On Wed, 31 Aug 2016, Jakub Narębski wrote:
>> W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:

>>> @@ -471,17 +487,20 @@ int sequencer_commit(const char *defmsg, struct 
>>> replay_opts *opts,
>>>  
>>> if (IS_REBASE_I()) {
>>> env = read_author_script();
>>> -   if (!env)
>>> +   if (!env) {
>>> +   const char *gpg_opt = gpg_sign_opt_quoted(opts);
>>> +
>>> return error("You have staged changes in your working "
>>> "tree. If these changes are meant to be\n"
>>> "squashed into the previous commit, run:\n\n"
>>> -   "  git commit --amend $gpg_sign_opt_quoted\n\n"
>>
>> How did this get expanded by error(), and why we want to replace
>> it if it works?

After writing this email, I got an idea on how it could work:
git-rebase script calls some C helper, which outputs above, and
output of this helper is eval'ed by script (with gpg_sign_opt_quoted
variable present in the environment)...

> 
> It did not work. It was a place-holder waiting for this patch ;-)
> 

... but it might have been simply copy'n'pasted from shell script
to C, literally.

>>
>>> +   "  git commit --amend %s\n\n"
>>> "If they are meant to go into a new commit, "
>>> "run:\n\n"
>>> -   "  git commit $gpg_sign_opt_quoted\n\n"
>>> +   "  git commit %s\n\n"
>>> "In both case, once you're done, continue "
>>> "with:\n\n"
>>> -   "  git rebase --continue\n");
>>> +   "  git rebase --continue\n", gpg_opt, gpg_opt);
>>
>> Instead of passing option twice, why not make use of %1$s (arg reordering),
>> that is
>>
>>   +  "  git commit --amend %1$s\n\n"
>> [...]
>>   +  "  git commit %1$s\n\n"
> 
> Cute. But would this not drive the l10ners insane?
> 

Shouldn't, as l10ners need to deal with arg reordering, because in different
languages the order of words might be different: %s %s in English may be
%2$s %1$s in other language, see example in
  https://www.gnu.org/software/gettext/manual/gettext.html#c_002dformat-Flag

Best,
-- 
Jakub Narębski

bitmap creation failed

[gjarms]

Hi Git Experts,

We have been exploring various ways to improve git cloning time, one among
them is using bitmap which is suppose to save time "counting objects".  but
i have problem creating bitmap since the repository contains 100's of pack
files. the bitmap file is not created when i use "git gc".

I have the following entries in my .gitconfig.

[pack]
packSizeLimit = 10m
writebitmaps = on
writeBitmapHashCache = on

If i just dont use "packSizeLimit = 10m", then bitmap is created just by
running git gc

Can you please make me understand ?, What i understood is that the bitmap is
created when there is a single pack file, but if i split it into multiple
pack file, the bitmap generation fails with the warning
"warning: disabling bitmap writing, as some objects are not being packed".

Regards,
Arumuga



--
View this message in context: 
http://git.661346.n2.nabble.com/bitmap-creation-failed-tp7657450.html
Sent from the git mailing list archive at Nabble.com.

Re: [PATCH 13/22] sequencer: remember the onelines when parsing the todo file

[Junio C Hamano]

Jakub Narębski  writes:

> I wonder how probable is situation where we save instruction sheet
> for interactive rebase, with shortened SHA-1, and during rebase
> shortened SHA-1 stops being unambiguous...

It is my understanding that the shortened ones are only for end-user
consumption.  The insn sheet internally uses fully expanded form for
this exact reason, and then abbreviated back at each step before the
updated one is presented to the end-user.  Uniqueness guarantee is
enforced with new objects created during each step taken into
account by doing it this way.

Re: [PATCH 13/22] sequencer: remember the onelines when parsing the todo file

[Jakub Narębski]

Hello,

W dniu 01.09.2016 o 15:12, Johannes Schindelin pisze:
> On Wed, 31 Aug 2016, Jakub Narębski wrote:
>> W dniu 31.08.2016 o 21:10, Junio C Hamano pisze:

>>> So I am not sure if we want a parsed commit there (I would not
>>> object if we kept the texual object name read from the file,
>>> though).  The "one sequencer to rule them all" may even have to say
>>> "now give name ':1' to the result of the previous operation" in one
>>> step and in another later step have an instruction "merge ':1'".
>>> When that happens, you cannot even pre-populate the commit object
>>> when the sequencer reads the file, as the commit has not yet been
>>> created at that point.
>>
>> True, --preserve-merges rebase is well, different.
> 
> It is mis-designed. And I can be that harsh because it was my design.
> 
> In the meantime I came up with a much better design, and implemented it as
> a shell script on top of rebase -i. Since shell scripts run like slow
> molasses, even more so on Windows, I have a loose plan to implement its
> functionality as a new --recreate-merges option, and to deprecate
> --preserve-merges when that new option works.
> 
> It needs to be a new option (not a --preserve-merges=v2) because it is a
> totally different beast. For starters, it does not need its own code path
> that overrides pick_one, as --preserve-merges does.

Better preserving for merges (with cleanly defined sematics)
would be certainly nice to have.

> But I get way ahead of myself. First we need to get these last few bits
> and pieces in place to accelerate (non --preserve-merges) rebase -i.

But it can wait, right.

-- 
Jakub Narębski

Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests

[Junio C Hamano]

Sverre Rabbelier  writes:

>>> I can't really recall, but I think it may have been related to me
>>> doing something like this:
>>> 1. Make a change, and start running tests (this takes a long time)
>>> 2. Notice a failure, start fixing it, leave tests running to find
>>> further failures
>>> 3. Finish fix, first tests are still running, start another run in a
>>> new terminal (possibly of just the one failed test I was fixing) to
>>> see if the fix worked.
>>>
>>> Without the pid, the second run would clobber the results from the first 
>>> run.
>>>
>> Would present-you disagree with stripping off the - suffix, based on
>> your recollections?
>
> No objections, I think it should be fine. If anyone uncovers a
> particularly compelling reason later on, it's only a commit away :).

OK, especially with the earlier observation made by Peff in the log
message:

... we can see that other files we write to test-results (like
*.exit and *.out) do _not_ have the PID included. So the
presence of the PID does not meaningfully allow one to store the
results from multiple runs anyway.

even if we wanted to, keeping the current code with suffix is not
sufficient, so I suspect it won't be just "a commit" away, but we
should be able to lose it for now.  Hopefully that would help making
Dscho's "what are the failed tests?" logic simpler.

Thanks.

Re: [PATCH 13/22] sequencer: remember the onelines when parsing the todo file

[Jakub Narębski]

W dniu 01.09.2016 o 11:37, Johannes Schindelin pisze:
> On Wed, 31 Aug 2016, Junio C Hamano wrote:
>> Jakub Narębski  writes:
>>
 diff --git a/sequencer.c b/sequencer.c
 index 06759d4..3398774 100644
 --- a/sequencer.c
 +++ b/sequencer.c
 @@ -709,6 +709,8 @@ static int read_and_refresh_cache(struct replay_opts 
 *opts)
  struct todo_item {
enum todo_command command;
struct commit *commit;
 +  const char *arg;
 +  int arg_len;
>>>
>>> Why 'arg', and not 'oneline', or 'subject'?
>>> I'm not saying it is bad name.
>>
>> I am not sure what the "commit" field of type "struct commit *" is
>> for.  It is not needed until it is the commit's turn to be picked or
>> reverted; if we end up stopping in the middle, parsing the commit
>> object for later steps will end up being wasted effort.
> 
> No, it won't be wasted effort, as we *validate* the todo script this way.
> And since we may very well need the info later (most rebases do not fail
> in the middle), we store it, too.

The question was (I think) whether we should do eager parsing of
commits, or whether we can do lazy parsing by postponing full parsing
"until it is the commit's turn to be picked or reverted", and possibly
when saving todo file.

I wonder how probable is situation where we save instruction sheet
for interactive rebase, with shortened SHA-1, and during rebase
shortened SHA-1 stops being unambiguous...

>[...] after parsing the todo_list, we will
> have to act on the information contained therein. For example we will have
> to cherry-pick some of the indicated commits (requiring a struct commit *
> for use in do_pick_commit()). Another example: we may need to determine
> the oneline for use in fixup!/squash! reordering.
> 
> So: keeping *that* aspect of the previous todo_list parsing, i.e. store a
> pointer to the already-parsed commit, is the right thing to do.

The above probably means that eager eval is better

Best,
-- 
Jakub Narębski

Re: `make profile-install` fails in 2.9.3

[Junio C Hamano]

Jeff King  writes:

> Yeah, I had a similar thought, but it just feels so hacky. Is there
> anything wrong with making this completely separate from the content
> update. I.e., just applying the pathspec to the index as a separate step
> and adding "+x" to each entry?
>
> This really is just a more convenient interface around "update-index
> --chmod", isn't it? We should be able to do the same thing it does.

Sure, the simplest and the most straight-forward way may look dumb,
but it would be the safest.

Re: [PATCH 10/22] sequencer: avoid completely different messages for different actions

[Jakub Narębski]

Hello Johannes

W dniu 01.09.2016 o 09:52, Johannes Schindelin pisze:
> On Wed, 31 Aug 2016, Jakub Narębski wrote:
>> CC-ed to Jiang Xin, L10N coordinator.
>> W dniu 29.08.2016 o 10:05, Johannes Schindelin pisze:

[...]
>>> -   /* Different translation strings for cherry-pick and revert */
>>> -   if (opts->action == REPLAY_PICK)
>>> -   error(_("Your local changes would be overwritten by 
>>> cherry-pick."));
>>> -   else
>>> -   error(_("Your local changes would be overwritten by revert."));
>>> +   error(_("Your local changes would be overwritten by %s."),
>>> +   action_name(opts));
>>
>> If I understand it correctly, it would make "revert" or "cherry-pick"
>> untranslated part of error message.  You would need to use translation
>> on the result with "_(action_name(opts))", you would have to mark
>> todo_command_strings elements for gettext lexicon with N_(...).
>>
>> I am rather against this change (see also below).
> 
> Okay.
> 
> Unfortunately, I have to focus on the correctness of the code at the
> moment (and Git for Windows does ship *without* translations for the time
> being anyway, mostly to save on space, but also because users complained).

Users complained about having translations, or not having easy way to
switch them or switch them off?

> 
> So I will take care of this after v2.10.0.
> 
> For the record, how is this supposed to be handled, in particular when I
> introduce a new action whose action_name(opts) will be "rebase -i"? Do I
> really need to repeat myself three times?

I think you should be able to mark strings to be translated,
without translating them at the time of definition,

  static const char *todo_command_strings[] = {
N_("pick"),
N_("revert")
  };

then translate at the point of use

error(_("Your local changes would be overwritten by %s."),
_(action_name(opts)));

I assume that action_name(opts) returns one of todo_command_strings.
If not, there should be array with possible actions.


Assuming that such lego l10n is preferable to multiple translations,
more free-formt.

-- 
Jakub Narębski

Re: Should "git symbolic-ref -d HEAD" be forbidden?

[Junio C Hamano]

Jeff King  writes:

> On Thu, Sep 01, 2016 at 02:08:08PM -0700, Junio C Hamano wrote:
>
>> I think we should.
>> 
>> t1401 expects to be able to, but if you really do it:
>> 
>>  $ cd /tmp
>>  $ git init throwaway
>> $ cd throwaway
>> $ git symbolic-ref -d HEAD
>> 
>> the setup machinery considers that you are no longer in a working
>> tree that is controlled by a repository at .git/ because .git/ is
>> no longer a valid repository, so you cannot even do
>> 
>>  $ git symbolic-ref HEAD refs/heads/master
>> 
>> to recover.
>
> Yes, I think we should, too. The same reasoning from afe5d3d (symbolic
> ref: refuse non-ref targets in HEAD, 2009-01-29) applies.

-- >8 --
Subject: symbolic-ref -d: do not allow removal of HEAD

If you delete the symbolic-ref HEAD from a repository, Git no longer
considers it valid, and even "git symbolic-ref HEAD refs/heads/master"
would not be able to recover from that state.

In the spirit similar to afe5d3d5 ("symbolic ref: refuse non-ref
targets in HEAD", 2009-01-29), forbid removal of HEAD.

Signed-off-by: Junio C Hamano 
---

 I decided against it for now for no good reason, other than I am a
 bit superstitious, but it may be a good idea to move these safety
 checks to delete_ref() and create_symref() in the longer term.

 builtin/symbolic-ref.c  |  2 ++
 t/t1401-symbolic-ref.sh | 19 ---
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
index 9c29a64..96eed94 100644
--- a/builtin/symbolic-ref.c
+++ b/builtin/symbolic-ref.c
@@ -56,6 +56,8 @@ int cmd_symbolic_ref(int argc, const char **argv, const char 
*prefix)
ret = check_symref(argv[0], 1, 0, 0);
if (ret)
die("Cannot delete %s, not a symbolic ref", argv[0]);
+   if (!strcmp(argv[0], "HEAD"))
+   die("deleting '%s' is not allowed", argv[0]);
return delete_ref(argv[0], NULL, REF_NODEREF);
}
 
diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh
index ca3fa40..5c30f94 100755
--- a/t/t1401-symbolic-ref.sh
+++ b/t/t1401-symbolic-ref.sh
@@ -33,18 +33,23 @@ test_expect_success 'symbolic-ref refuses bare sha1' '
 '
 reset_to_sane
 
-test_expect_success 'symbolic-ref deletes HEAD' '
-   git symbolic-ref -d HEAD &&
+test_expect_success 'HEAD cannot be removed' '
+   test_must_fail git symbolic-ref -d HEAD
+'
+
+test_expect_success 'symbolic-ref can be deleted' '
+   git symbolic-ref NOTHEAD refs/heads/foo &&
+   git symbolic-ref -d NOTHEAD &&
test_path_is_file .git/refs/heads/foo &&
-   test_path_is_missing .git/HEAD
+   test_path_is_missing .git/NOTHEAD
 '
 reset_to_sane
 
-test_expect_success 'symbolic-ref deletes dangling HEAD' '
-   git symbolic-ref HEAD refs/heads/missing &&
-   git symbolic-ref -d HEAD &&
+test_expect_success 'symbolic-ref can delete dangling symref' '
+   git symbolic-ref NOTHEAD refs/heads/missing &&
+   git symbolic-ref -d NOTHEAD &&
test_path_is_missing .git/refs/heads/missing &&
-   test_path_is_missing .git/HEAD
+   test_path_is_missing .git/NOTHEAD
 '
 reset_to_sane
 

Re: Are --first-parent and --ancestry-path compatible rev-list options?

[Junio C Hamano]

"Philip Oakley"  writes:

> The user question was, given a commit 'J', and a future commit 'H'
> (typically a branch tip such as 'master'), find those commits that are
> :
> A) merges
> B) on the first parent DAG chain of the future commit 'H'
> C) children of the given commit 'J'

The answer then is that there is no such single step operation.

In the picture, if D or E were a merge from a side branch that does
not have anything to do with 'J', "log --first-parent --merges" will
not exclude it (i.e. C won't be fulfilled by --first-parent --merges).

Re: `make profile-install` fails in 2.9.3

[Jeff King]

On Thu, Sep 01, 2016 at 03:16:38PM -0700, Junio C Hamano wrote:

> Which means that piggybacking this on the "run 'git diff' limited to
> the pathspec to find the paths that needs updating" logic usually
> done in "git add" can not be reused [*1*].
> 
> What was I thinking while reviewing the patch X-<.  Sigh.
> 
> 
> [Footnote]
> 
> *1* I guess we _could_, by first flipping all the regular file
> blob's executable bit for paths that are inside the pathspec and
> then by running "git diff" against that modified index, limited
> to the pathspec, to find the paths that need to be added.
> 
> It sounds ugly, but may conceptually be cleaner.  We first start
> from an ideal end-result, and then re-hash what needs to be
> updated to match the ideal.

Yeah, I had a similar thought, but it just feels so hacky. Is there
anything wrong with making this completely separate from the content
update. I.e., just applying the pathspec to the index as a separate step
and adding "+x" to each entry?

This really is just a more convenient interface around "update-index
--chmod", isn't it? We should be able to do the same thing it does.

-Peff

Re: `make profile-install` fails in 2.9.3

[Junio C Hamano]

Jeff King  writes:

> Yeah, I think we should _always_ act on the --chmod, no matter if the
> file is racy or not, or whether it has a content change or not. I.e.,
> the race is not the problem, but rather the behavior of 4e55ed32. Your
> second proposal there sounds more like the right approach.

Yeah, you two are absolutely right.  The second "git add --chmod=+x"
in

$ git add .
$ git add --chmod=+x .

should still find _all_ the non-executable paths and flip their
executable bit in the index, making them all up-to-date in the
index.

Which means that piggybacking this on the "run 'git diff' limited to
the pathspec to find the paths that needs updating" logic usually
done in "git add" can not be reused [*1*].

What was I thinking while reviewing the patch X-<.  Sigh.


[Footnote]

*1* I guess we _could_, by first flipping all the regular file
blob's executable bit for paths that are inside the pathspec and
then by running "git diff" against that modified index, limited
to the pathspec, to find the paths that need to be added.

It sounds ugly, but may conceptually be cleaner.  We first start
from an ideal end-result, and then re-hash what needs to be
updated to match the ideal.

Re: [PATCH v13 06/14] apply: make it possible to silently apply

[Stefan Beller]

On Thu, Sep 1, 2016 at 1:01 AM, Christian Couder
 wrote:
> On Thu, Sep 1, 2016 at 12:07 AM, Stefan Beller  wrote:
>>> Printing on stdout, and calls to warning() or error() are not
>>> taken care of in this patch, as that will be done in following
>>> patches.
>>
>>> -   if (state->apply_verbosely)
>>> +   if (state->apply_verbosity > verbosity_normal)
>>> error(_("while searching for:\n%.*s"),
>>>   (int)(old - oldlines), oldlines);
>>
>> But this is an error(..) ?
>
> Do you mean that it was a bug in the original code to print this error
> only in verbose mode?

Oh never mind.

I meant to point out the inconsistency between the commit message, that
said: "error() are not taken care of in this patch" and modifying a
condition for
an error call. However we need to fix them as you renamed apply_verbosely
to apply_verbosity and made it an enum. So I spoke too early.

>
> Anyway I don't think such a refactoring is needed.

great :)

Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests

[Sverre Rabbelier]

On Thu, Sep 1, 2016 at 1:27 AM, Johannes Schindelin
 wrote:
> On Wed, 31 Aug 2016, Sverre Rabbelier wrote:
>> On Wed, Aug 31, 2016 at 3:36 AM Johannes Schindelin
>>  wrote:
>> > On Tue, 30 Aug 2016, Junio C Hamano wrote:
>> > > Jeff King  writes:
>> > > > Hmm, interesting. Your approach seems reasonable, but I have to wonder
>> > > > if writing the pid in the first place is sane.
>> > > >
>> > > > I started to write up my reasoning in this email, but realized it was
>> > > > rapidly becoming the content of a commit message. So here is that
>> > > > commit.
>> > >
>> > > Sounds sensible; if this makes Dscho's "which ones failed in the
>> > > previous run" simpler, that is even better ;-)
>> >
>> > I did not have the time to dig further before now. There must have been a
>> > good reason why we append the PID.
>> >
>> > Sverre, you added that code in 2d84e9f (Modify test-lib.sh to output stats
>> > to t/test-results/*, 2008-06-08): any idea why the - suffix was
>> > needed?
>>
>> I can't really recall, but I think it may have been related to me
>> doing something like this:
>> 1. Make a change, and start running tests (this takes a long time)
>> 2. Notice a failure, start fixing it, leave tests running to find
>> further failures
>> 3. Finish fix, first tests are still running, start another run in a
>> new terminal (possibly of just the one failed test I was fixing) to
>> see if the fix worked.
>>
>> Without the pid, the second run would clobber the results from the first run.
>>
>>
>> If only past-me was more rigorous about writing good commit messages :P.
>
> :-)
>
> Would present-you disagree with stripping off the - suffix, based on
> your recollections?

No objections, I think it should be fine. If anyone uncovers a
particularly compelling reason later on, it's only a commit away :).

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 09/22] sequencer: completely revamp the "todo" script parsing

[Jakub Narębski]

Hello Johannes,

W dniu 01.09.2016 o 09:49, Johannes Schindelin pisze:
> On Wed, 31 Aug 2016, Jakub Narębski wrote: 
>> W dniu 29.08.2016 o 10:05, Johannes Schindelin pisze:
[...]

>> Hmmm... commit_list is, as defined in commit.h, a linked list.
> 
> That is the most prominent reason why the rest is not a mindless
> conversion from commit_list to todo_list.
> 
> And we need todo_list as an array, because we need to be able to peek, or
> even move, backwards from the current command.
> 
>> Here todo_list uses growable array implementation of list.  Which
>> is I guess better on current CPU architecture, with slow memory,
>> limited-size caches, and adjacency prefetching.
> 
> That is not the reason that an array is used here. The array allows us
> much more flexibility.

It would be nice if this reasoning (behind the change from linked list
to growable array) was mentioned in appropriate commit message, and
perhaps also in the cover letter for the series.  It is IMVHO quite
important information (that you thought obvious).

> 
> One of the major performance improvements will come at the very end, for
> example: the reordering of the fixup!/squash! lines. And that would be a
> *major* pain to do if the todo_list were still a linked list.

Actually deletion from and insertion into single linked list are
not that hard, and O(1) after finding place, O(N) with finding
included.  Moving elements in array is O(N),... and arguably a bit
simpler - but at high level, with appropriate primitives, they are
about the same.

Yes, array is easier for permutation and reordering.

>>> +struct todo_item *append_todo(struct todo_list *todo_list)
>>
>> Errr... I don't quite understand the name of this function.
>> What are you appending here to the todo_list?
> 
> A new item.
> 
>> Compare string_list_append() and string_list_append_nodup(),
>> where the second parameter is item to append.
> 
> Yes, that is correct. In the case of a todo_item, things are a lot more
> complicated, though. Some of the values have to be determined tediously
> (such as the offset and length of the oneline after the "pick "
> command). I just put those values directly into the newly allocated item,
> is all.

I would expect sth_append command to take a list (or other collection),
an element, and return [modified] collection with the new element added.
Such API would require temporary variable in caller and memcopy in the
sth_append() function.

This is not it.  It creates a new element, expanding a list (a collection),
and then expose this element.  Which spares us memcopy... on non-critical
path.

I don't know how to name operation "grow list and return new element".
But "append" it is not.
 
>>> +   ALLOC_GROW(todo_list->items, todo_list->nr + 1, todo_list->alloc);
>>> +   return todo_list->items + todo_list->nr++;
>>>  }
>>>  
>>> -static struct commit *parse_insn_line(char *bol, char *eol, struct 
>>> replay_opts *opts)
>>> +static int parse_insn_line(struct todo_item *item, const char *bol, char 
>>> *eol)
>>
>> Why the change of return type?  
> 
> Because it makes no sense to return a commit here because not all commands
> are about commits (think rebase -i's `exec`). It makes tons of sense to
> return an error condition, though.

All right.

I have not checked / not remember what caller of parse_insn_line() used
it's return value for.  I guess that we save it into todo_item, moving
that operation from caller to callee.

>> Why now struct todo_item is first when struct replay_opts was last?
> 
> Those play very, very different roles.
> 
> The opts parameter used to provide parse_insn_line() with enough
> information to complain loudly when the overall command was not identical
> to the parsed command.
> 
> The item parameter is a receptacle for the parsed data. It will contain
> the pointer to the commit that was previously returned, if any. But it
> will also contain much more information, such as the command, the oneline,
> the offset in the buffer, etc etc
> 
> So "opts" was an "in" parameter while "item" is an "out" one. Apples and
> oranges.

All right.  Good explanation.  And the one that is too low-level
detail to put in the commit message, I think.

>>> -   end_of_object_name = bol + strcspn(bol, " \t\n");
>>> +   end_of_object_name = (char *) bol + strcspn(bol, " \t\n");
>>
>> Why is this cast needed?
> 
> Because bol is a "const char *" and we need to put "NUL" temporarily to
> *end_of_object_name:

Would compiler complain without this const'ness-stripping cast?

> 
>>> saved = *end_of_object_name;
>>> *end_of_object_name = '\0';
>>> status = get_sha1(bol, commit_sha1);
>>> *end_of_object_name = saved;
> 
> Technically, this would have made a fine excuse to teach get_sha1() a mode
> where it expects a length parameter instead of relying on a NUL-terminated
> string.
> 
> Practically, such fine excuses cost me months in this rebase--helper
> project already, and I need to protect my time better.

Put it in TODO 

Re: `make profile-install` fails in 2.9.3

[Jeff King]

On Thu, Sep 01, 2016 at 09:07:00PM +0100, Thomas Gummerer wrote:

> > Related problem: `t3700-add.sh` currently fails in 2.9.3. I can
> > provide more debug information if you don't already know this problem.
> 
> I noticed this problem as well, when I'm compiling with USE_NSEC = 1
> in my config.mak.

I can replicate this even without USE_NSEC with my stress-tester[1].
That makes sense why it would show up with the profiling run; git runs
slower and therefore increases the chances of crossing the 1-second
boundary and losing the race.

[1] https://github.com/peff/git/blob/meta/stress

> Tracking this problem down a bit, it happens because the --chmod=[+-]x
> option introduced in 4e55ed32 ("add: add --chmod=+x / --chmod=-x
> options") only works if the file on disk is modified.  When the test
> was changed to work on one single file, instead of doing chmod=+x on
> one file and chmod=-x on another file in b38ab197c ("t3700: merge two
> tests into one"), this test started breaking when the mtime of the
> file and the index file weren't the same (in other words, if the file
> was not racily clean and thus was not smudged).

That certainly sounds buggy. A less racy way of verifying this is just:

  # guarantee not-racy state
  echo content >file
  test-chmtime -60 file
  git add file

  # now check --chmod; file will still be 100644!
  git add --chmod=+x file
  git ls-files -s

> One possible fix for the test is to smudge the entry as showed below,
> though I'm not sure it's the right fix.  The other way I can think of
> is to change the file in the index regardless of whether the file was
> changed in some other way before issuing the git add command, as that
> might fit the user expectation better.  Thoughts?

Yeah, I think we should _always_ act on the --chmod, no matter if the
file is racy or not, or whether it has a content change or not. I.e.,
the race is not the problem, but rather the behavior of 4e55ed32. Your
second proposal there sounds more like the right approach.

-Peff

Re: git submodules implementation question

[Uma Srinivasan]

>>> The final version needs to be accompanied with tests to show the
>>> effect of this change for callers.  A test would set up a top-level
>>> and submodule, deliberately break submodule/.git/ repository and
>>> show what breaks and how without this change.

Agreed!

The repo where the original problem surfaced is huge and in fact "git
status" appears to go into an infinite loop forking a zillion
processes on the system. If the dev system is small, it brings it to a
standstill. However, the good news is that I could build myself a
smaller reproducer by doing the following:

1) mkdir sm_test
2) cd sm_test
3) git clone git://git.mysociety.org/commonlib commonlib
4) git init
5) git submodule add ./commonlib/
6) cd commonlib/.git
7) rm -f all files

After this "git status" will fork several thousand processes but it
will ultimately fail as it runs into the path length max limit. I
still don't know why this doesn't happen with my original problem
repo. Perhaps I have to let it run overnight or perhaps it runs into
other system limitations before then

Anyway, with the fix "git status" fails quickly both in my reproducer
(and original repo) with the following message.
fatal: Not a git repository: '.git'
fatal: 'git status --porcelain' failed in submodule commonlib

Hope this helps.

Uma


On Thu, Sep 1, 2016 at 12:19 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>>> The final version needs to be accompanied with tests to show the
>>> effect of this change for callers.  A test would set up a top-level
>>> and submodule, deliberately break submodule/.git/ repository and
>>> show what breaks and how without this change.
>>
>> Tests are really good at providing this context as well, or to communicate
>> the actual underlying problem, which is not quite clear to me.
>> That is why I refrained from jumping into the discussion as I think the
>> first few emails were dropped from the mailing list and I am missing context.
>
> I do not know where you started reading, but the gist of it is that
> submodule.c spawns subprocess to run in the submodule's context by
> assuming that chdir'ing into the  of the submodule and running
> it (i.e. cp.dir set to  to drive start_command()) is
> sufficient.  When /.git (either it is a directory itself or it
> points at a directory in .git/module/ in the superproject) is
> a corrupt repository, running "git -C  command" would try to
> auto-detect the repository, because it thinks /.git is not a
> repository and it thinks it is not at the top-level of the working
> tree, and instead finds the repository of the top-level, which is
> almost never what we want.
>

Re: [PATCH] make dist: allow using an installed version of git

[Dennis Kaarsemaker]

On do, 2016-09-01 at 10:43 -0700, Junio C Hamano wrote:
> Dennis Kaarsemaker  writes:
> 
> > b1de9de2 back in 2005 ensured that we could create a tarball with 'make
> > dist' even if git wasn't installed yet. These days however, chances are
> > higher that a git version is available. Add a config.mak knob to allow
> > people to choose to use the installed version of git to create the
> > tarball and avoid the overhead of building git-archive.
> 
> Thanks, but not interested.

Pity. Would save me quite a bit of tarball build time.

> We do not know what vintage of "git" happens to be installed on the
> platform, but we know how "git archive" we ship with the source
> ought to behave.

That's why I didn't want to make it the default, but merely make it an
available option for saving some build time for people who know their
git is up-to-date enough.

D.

Re: git submodules implementation question

[Stefan Beller]

> The final version needs to be accompanied with tests to show the
> effect of this change for callers.  A test would set up a top-level
> and submodule, deliberately break submodule/.git/ repository and
> show what breaks and how without this change.

Tests are really good at providing this context as well, or to communicate
the actual underlying problem, which is not quite clear to me.
That is why I refrained from jumping into the discussion as I think the
first few emails were dropped from the mailing list and I am missing context.

>
> So it is a bit more involved than just a single liner patch with one
> paragraph log message.  I may be able to find or make some time
> after I tag the 2.10 final this weekend to do so myself.

Thanks,
Stefan

Re: implement a stable 'Last updated' in Documentation

[Jeff King]

On Thu, Sep 01, 2016 at 04:37:36PM +0200, Olaf Hering wrote:

> Hey, asciidoc made a move, so this patch is good to go:
> https://github.com/asciidoc/asciidoc/pull/9

Sine this thread is 18 months old, I needed some recap to remember what
we were talking about. :)

It's here:

  http://public-inbox.org/git/20150126172409.ga15...@aepfle.de/T/#u

and the gist of it is that we'd like to drop the "Last updated" footer
from the HTML version of the manpages, but older versions of asciidoc
did not provide a mechanism.

The patch you quoted adds "footer-style=none", which would do the trick.
But I have two open questions:

  1. What does this do on older versions of asciidoc? Is it silently
 ignored (ok), or does it generate an error (bad)?

  2. This covers the HTML versions, but not the roff manpages (which
 are generated by docbook). Do we have a way to tweak the date in
 the latter?

 I don't think that's necessarily a requirement for this patch, but
 it is worth thinking about at the same time.

Assuming the answer to (1) is "ok" and (2) is "no, but it's hard because
docbook is scary, so let's punt", then somebody needs to write up the
commit message and send the actual patch to the list. Would you like to
try that?

-Peff

Re: Should "git symbolic-ref -d HEAD" be forbidden?

[Jeff King]

On Thu, Sep 01, 2016 at 02:08:08PM -0700, Junio C Hamano wrote:

> I think we should.
> 
> t1401 expects to be able to, but if you really do it:
> 
>   $ cd /tmp
>   $ git init throwaway
> $ cd throwaway
> $ git symbolic-ref -d HEAD
> 
> the setup machinery considers that you are no longer in a working
> tree that is controlled by a repository at .git/ because .git/ is
> no longer a valid repository, so you cannot even do
> 
>   $ git symbolic-ref HEAD refs/heads/master
> 
> to recover.

Yes, I think we should, too. The same reasoning from afe5d3d (symbolic
ref: refuse non-ref targets in HEAD, 2009-01-29) applies.

-Peff

wall socket extender

[Bella]

Dear Sir/Madam,
 Hello,this is Bella from Newmany. If you need socket related products, please 
feel free to contact me. Looking forward to hear from you.


Bella Zhang
杭州新万利电子有限公司
Hangzhou Newmany Electronics Co., Ltd.
Tel: 86-571-64701616
Fax: 86-571-64701615
http://newmany.en.alibaba.com
Whatsapp:0086 18668180515
Skype:bella3794

Re: [PATCH 0/2] squelch some "gcc -O3 -Wmaybe-uninitialized" warnings

[Jeff King]

On Wed, Aug 31, 2016 at 12:55:01PM -0700, Junio C Hamano wrote:

> Interesting.  Here is for "gcc -Os" on top to appease gcc 4.8.4 that
> I probably am NOT going to apply.  These are all false positives.
> 
> The ones on config.c is the most curious as these two "ret" needs a
> false initialization, but the one that comes after them
> git_config_ulong() that has the same code structure does not get any
> warning, which made absolutely no sense to me.

Yeah, I'd agree that is really odd. I wondered if perhaps the signedness
of the argument mattered (e.g., if we were somehow provoking undefined
behavior which caused the compiler to make some assumption), but I just
don't see it.

>  builtin/update-index.c | 2 +-
>  config.c   | 4 ++--
>  diff.c | 2 +-
>  fast-import.c  | 1 +
>  4 files changed, 5 insertions(+), 4 deletions(-)

FWIW, all but the fast-import one have gone away in gcc 6.2.0 (using
-Os).

For that one:

> diff --git a/fast-import.c b/fast-import.c
> index bf53ac9..abc4519 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -1377,6 +1377,7 @@ static const char *get_mode(const char *str, uint16_t 
> *modep)
>   unsigned char c;
>   uint16_t mode = 0;
>  
> + *modep = 0;
>   while ((c = *str++) != ' ') {
>   if (c < '0' || c > '7')
>   return NULL;

The complaint actually comes from the caller, who doesn't realize that
modep will be set.

It pretty clearly seems to be a false positive, but I don't understand
it. If get_mode() is not inlined (or otherwise examined when considering
the caller), then it presumably should be treated as a block box that we
assume sets "modep".  And if it is inlined, then it's pretty obvious
that "modep" is initialized in any code path that does not return NULL,
and we have:

p = get_mode(p, );
if (!p)
die("Corrupt mode: %s", command_buf.buf);

in the caller (and "die" is marked as NORETURN). So it seems like a
pretty easy case to get right, and one that the compiler presumably gets
right elsewhere (otherwise we'd have a lot more of these false
positives).

Weird.

-Peff

Re: git submodules implementation question

[Stefan Beller]

On Thu, Sep 1, 2016 at 1:21 PM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> Stefan Beller  writes:
>>
 The final version needs to be accompanied with tests to show the
 effect of this change for callers.  A test would set up a top-level
 and submodule, deliberately break submodule/.git/ repository and
 show what breaks and how without this change.
>>>
>>> Tests are really good at providing this context as well, or to communicate
>>> the actual underlying problem, which is not quite clear to me.
>>> That is why I refrained from jumping into the discussion as I think the
>>> first few emails were dropped from the mailing list and I am missing 
>>> context.
>>
>> I do not know where you started reading, but the gist of it is that
>> submodule.c spawns subprocess to run in the submodule's context by
>> assuming that chdir'ing into the  of the submodule and running
>> it (i.e. cp.dir set to  to drive start_command()) is
>> sufficient.  When /.git (either it is a directory itself or it
>> points at a directory in .git/module/ in the superproject) is
>> a corrupt repository, running "git -C  command" would try to
>> auto-detect the repository, because it thinks /.git is not a
>> repository and it thinks it is not at the top-level of the working
>> tree, and instead finds the repository of the top-level, which is
>> almost never what we want.
>
> This is with a test that covers the call in get_next_submodule() for
> the parallel fetch callback.  I think many of the codepaths will end
> up recursing forever the same way without the fix in a submodule
> repository that is broken in a similar way, but I didn't check, so
> I do not consider this to be completed.

Oh I see. That seems like a nasty bug.

>
> -- >8 --
> Subject: submodule: avoid auto-discovery in prepare_submodule_repo_env()
>
> The function is used to set up the environment variable used in a
> subprocess we spawn in a submodule directory.  The callers set up a
> child_process structure, find the working tree path of one submodule
> and set .dir field to it, and then use start_command() API to spawn
> the subprocess like "status", "fetch", etc.
>
> When this happens, we expect that the ".git" (either a directory or
> a gitfile that points at the real location) in the current working
> directory of the subprocess MUST be the repository for the submodule.
>
> If this ".git" thing is a corrupt repository, however, because
> prepare_submodule_repo_env() unsets GIT_DIR and GIT_WORK_TREE, the
> subprocess will see ".git", thinks it is not a repository, and
> attempt to find one by going up, likely to end up in finding the
> repository of the superproject.  In some codepaths, this will cause
> a command run with the "--recurse-submodules" option to recurse
> forever.
>
> By exporting GIT_DIR=.git, disable the auto-discovery logic in the
> subprocess, which would instead stop it and report an error.

and GIT_DIR=.git works for both .git files as well as the old fashioned way,
with the submodule repository at .git/, although that is not really documented.

>
> Not-signed-off-yet.
> ---
>  submodule.c |  1 +
>  t/t5526-fetch-submodules.sh | 29 +
>  2 files changed, 30 insertions(+)
>
> diff --git a/submodule.c b/submodule.c
> index 1b5cdfb..e8258f0 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1160,4 +1160,5 @@ void prepare_submodule_repo_env(struct argv_array *out)
> if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
> argv_array_push(out, *var);
> }
> +   argv_array_push(out, "GIT_DIR=.git");
>  }
> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> index 954d0e4..b2dee30 100755
> --- a/t/t5526-fetch-submodules.sh
> +++ b/t/t5526-fetch-submodules.sh
> @@ -485,4 +485,33 @@ test_expect_success 'fetching submodules respects 
> parallel settings' '
> )
>  '
>
> +test_expect_success 'fetching submodule into a broken repository' '
> +   # Prepare src and src/sub nested in it
> +   git init src &&
> +   (
> +   cd src &&
> +   git init sub &&
> +   git -C sub commit --allow-empty -m "initial in sub" &&
> +   git submodule add -- ./sub sub &&
> +   git commit -m "initial in top"
> +   ) &&

This is not needed, as setup() set up some repositories for you.

> +
> +   # Clone the old-fashoned way
> +   git clone src dst &&

if you don't go with your own setup, maybe:

git clone . dst
git -C dst clone ../submodule submodule

# and further down s/sub/submodule/

> +   git -C dst clone ../src/sub sub &&
> +
> +   # Make sure that old-fashoned layout is still supported

fashioned

> +   git -C dst status &&
> +
> +   # Recursive-fetch works fine
> +   git -C dst fetch --recurse-submodules &&
> +
> +   # Break the receiving submodule
> +   

Re: git submodules implementation question

[Junio C Hamano]

Uma Srinivasan  writes:

> Yes, this one line fix addresses my problem.
>
> So, what is the next step? Will someone submit a patch or should I?
> Please note that I've never submitted a patch before, but I don't mind
> learning how to.

The final version needs to be accompanied with tests to show the
effect of this change for callers.  A test would set up a top-level
and submodule, deliberately break submodule/.git/ repository and
show what breaks and how without this change.

For example, there is a call in ok_to_remove_submodule() to run "git
status" in the submodule directory.  A test writer needs to find who
calls ok_to_remove_submodule() in what codepath (e.g. builtin/rm.c).
Then after figuring out under what condition the check is triggered,
write a test that runs "git rm" in a way that this change makes a
difference.  Hopefully, the test will fail without this change, and
will pass with this change.

So it is a bit more involved than just a single liner patch with one
paragraph log message.  I may be able to find or make some time
after I tag the 2.10 final this weekend to do so myself.

>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -1160,4 +1160,5 @@ void prepare_submodule_repo_env(struct argv_array *out)
>> if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
>> argv_array_push(out, *var);
>> }
>> +   argv_array_push(out, "GIT_DIR=.git");
>>  }
>>

Re: Are --first-parent and --ancestry-path compatible rev-list options?

[Junio C Hamano]

"Philip Oakley"  writes:

> From: "Junio C Hamano" 
>> "Philip Oakley"  writes:
>>
>>> The commit graph. We are looking for F based on knowing J.
>>>
>>> . A - B - C - D -- E -- F -- G - H<-first parent, --merges (C,F,H)
>>> .  \  |  /\//
>>> .   Z |   //
>>> . |   |   |   /
>>> .  \   \ /   /
>>> .   I -[J]- K - L - M <-since J, children of J
>>> .\ /
>>> . N - O - P
>>
>> I think these two operations are fundamentally incompatible.
>
> If I run them independently, they both find the desired INTERESTED
> commit, hence the expectation that together they will still find that
> commit as an intersection between the two sets.
>
>>
>> Because the first-parent traversal is what the name says, i.e.,
>> forbids the positive side of revision traversal to stray into side
>> branches, the positive side of a traversal that begins at H will not
>> see M, L and K.
>
> But it does see F the ultimately desired commit.

You are doing --merges --first-parent, right?  Traversing only the
first-parent chain on the positive side, while excluding J's
ancestor by traversing the negative side without being limited to
the first-parent chain, would paint B and its ancestors as
uninteresting on the first-parent chain, so among H, G, F, E, D and
C, which are the survivors on the first-parent chain that are still
not UNINTERESTIN, the last one you would find that is a merge is F.

So I do not see any room for "But" to come in here...

Should "git symbolic-ref -d HEAD" be forbidden?

[Junio C Hamano]

I think we should.

t1401 expects to be able to, but if you really do it:

$ cd /tmp
$ git init throwaway
$ cd throwaway
$ git symbolic-ref -d HEAD

the setup machinery considers that you are no longer in a working
tree that is controlled by a repository at .git/ because .git/ is
no longer a valid repository, so you cannot even do

$ git symbolic-ref HEAD refs/heads/master

to recover.

Re: git submodules implementation question

[Junio C Hamano]

Stefan Beller  writes:

>> +test_expect_success 'fetching submodule into a broken repository' '
>> +   # Prepare src and src/sub nested in it
>> +   git init src &&
>> +   (
>> +   cd src &&
>> +   git init sub &&
>> +   git -C sub commit --allow-empty -m "initial in sub" &&
>> +   git submodule add -- ./sub sub &&
>> +   git commit -m "initial in top"
>> +   ) &&
>
> This is not needed, as setup() set up some repositories for you.

I didn't want any random cruft left behind in the top-level by
previous tests, so this is very much deliberate.

Re: [PATCH 13/22] sequencer: remember the onelines when parsing the todo file

[Junio C Hamano]

Johannes Schindelin  writes:

>> though).  The "one sequencer to rule them all" may even have to say
>> "now give name ':1' to the result of the previous operation" in one
>> step and in another later step have an instruction "merge ':1'".
>> When that happens, you cannot even pre-populate the commit object
>> when the sequencer reads the file, as the commit has not yet been
>> created at that point.
>
> These considerations are pretty hypothetical. I would even place a bet
> that we will *never* have ":1" as names, not if I have anything to say...
> ;-)

If you can always work with pre-existing commit, then you can
validate all object references that appear in the instructions
upfront.

I was sort of expecting that, when you do the preserve-merges mode
of "rebase -i", you would need to jump around, doing "we have
reconstructed the side branch on a new 'onto', let's give the result
this temporary name ':1', and then switch to the trunk (which would
call for 'reset ' instruction) and merge that thing (which
would be 'merge :1' or perhaps called 'pick :1')", and at that point
you no longer validate the object references upfront.

If you do not have to have such a "mark this point" and a "refer to
that point we previously marked", then I agree that you should be
able to pre-validate and keep the result in the structure.

Re: git submodules implementation question

[Junio C Hamano]

Junio C Hamano  writes:

If we add

# "git diff" should terminate with an error.
# NOTE: without fix this will recurse forever!
test_must_fail git -C dst diff &&

after breaking the repository, we can also see "git diff" recurse
forever, because it wants to know if "sub" submodule is modified,
attempts to run "git status" in there, and ends up running that
command in the context of the parent repository.

I am tempted to cheat and commit this, even though this test is no
longer about fetching submodules.

-- >8 --
[PATCH] submodule: avoid auto-discovery in prepare_submodule_repo_env()

The function is used to set up the environment variable used in a
subprocess we spawn in a submodule directory.  The callers set up a
child_process structure, find the working tree path of one submodule
and set .dir field to it, and then use start_command() API to spawn
the subprocess like "status", "fetch", etc.

When this happens, we expect that the ".git" (either a directory or
a gitfile that points at the real location) in the current working
directory of the subprocess MUST be the repository for the submodule.

If this ".git" thing is a corrupt repository, however, because
prepare_submodule_repo_env() unsets GIT_DIR and GIT_WORK_TREE, the
subprocess will see ".git", thinks it is not a repository, and
attempt to find one by going up, likely to end up in finding the
repository of the superproject.  In some codepaths, this will cause
a command run with the "--recurse-submodules" option to recurse
forever.

By exporting GIT_DIR=.git, disable the auto-discovery logic in the
subprocess, which would instead stop it and report an error.

The test illustrates existing problems in a few callsites of this
function.  Without this fix, "git fetch --recurse-submodules", "git
status" and "git diff" keep recursing forever.

Signed-off-by: Junio C Hamano 
---
 submodule.c |  1 +
 t/t5526-fetch-submodules.sh | 35 +++
 2 files changed, 36 insertions(+)

diff --git a/submodule.c b/submodule.c
index 4532b11..2801fbb 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1160,4 +1160,5 @@ void prepare_submodule_repo_env(struct argv_array *out)
if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
argv_array_push(out, *var);
}
+   argv_array_push(out, "GIT_DIR=.git");
 }
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 954d0e4..f3b0a8d 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -485,4 +485,39 @@ test_expect_success 'fetching submodules respects parallel 
settings' '
)
 '
 
+test_expect_success 'fetching submodule into a broken repository' '
+   # Prepare src and src/sub nested in it
+   git init src &&
+   (
+   cd src &&
+   git init sub &&
+   git -C sub commit --allow-empty -m "initial in sub" &&
+   git submodule add -- ./sub sub &&
+   git commit -m "initial in top"
+   ) &&
+
+   # Clone the old-fashoned way
+   git clone src dst &&
+   git -C dst clone ../src/sub sub &&
+
+   # Make sure that old-fashoned layout is still supported
+   git -C dst status &&
+
+   # "diff" would find no change
+   git -C dst diff --exit-code &&
+
+   # Recursive-fetch works fine
+   git -C dst fetch --recurse-submodules &&
+
+   # Break the receiving submodule
+   rm -f dst/sub/.git/HEAD &&
+
+   # NOTE: without the fix the following tests will recurse forever!
+   # They should terminate with an error.
+
+   test_must_fail git -C dst status &&
+   test_must_fail git -C dst diff &&
+   test_must_fail git -C dst fetch --recurse-submodules
+'
+
 test_done
-- 
2.10.0-rc2-314-g775ea9a

Re: git submodules implementation question

[Junio C Hamano]

Uma Srinivasan  writes:

> Anyway, with the fix "git status" fails quickly both in my reproducer
> (and original repo) with the following message.
> fatal: Not a git repository: '.git'
> fatal: 'git status --porcelain' failed in submodule commonlib

Thanks, that is exactly what I wanted to see as an outcome when I
did the one-liner patch.

Re: [PATCH v13 00/14] libify apply and use lib in am, part 3

[Junio C Hamano]

Christian Couder  writes:

> Following Stefan's review, it looks like I will need to resend at
> least 02/14, 10/14 and 14/14.
> What do you prefer me to resend:
> 1) all the last 40 or so patches
> 2) the last 14 patches
> 3) only the few patches that changed

If this reroll is to be the candidate to be the final one, I'd
prefer to see 1 to give an easy access to more sets of eyes, but if
you just send them without giving these patches one more read-over
before sending them out, it is not as valuable as it would be.

I think 2 is of the least value.

If you do 3., pointing people at where the remainder of the series
can be found is necessary.

Re: git submodules implementation question

[Junio C Hamano]

Junio C Hamano  writes:

> Stefan Beller  writes:
>
>>> The final version needs to be accompanied with tests to show the
>>> effect of this change for callers.  A test would set up a top-level
>>> and submodule, deliberately break submodule/.git/ repository and
>>> show what breaks and how without this change.
>>
>> Tests are really good at providing this context as well, or to communicate
>> the actual underlying problem, which is not quite clear to me.
>> That is why I refrained from jumping into the discussion as I think the
>> first few emails were dropped from the mailing list and I am missing context.
>
> I do not know where you started reading, but the gist of it is that
> submodule.c spawns subprocess to run in the submodule's context by
> assuming that chdir'ing into the  of the submodule and running
> it (i.e. cp.dir set to  to drive start_command()) is
> sufficient.  When /.git (either it is a directory itself or it
> points at a directory in .git/module/ in the superproject) is
> a corrupt repository, running "git -C  command" would try to
> auto-detect the repository, because it thinks /.git is not a
> repository and it thinks it is not at the top-level of the working
> tree, and instead finds the repository of the top-level, which is
> almost never what we want.

This is with a test that covers the call in get_next_submodule() for
the parallel fetch callback.  I think many of the codepaths will end
up recursing forever the same way without the fix in a submodule
repository that is broken in a similar way, but I didn't check, so
I do not consider this to be completed.

-- >8 --
Subject: submodule: avoid auto-discovery in prepare_submodule_repo_env()

The function is used to set up the environment variable used in a
subprocess we spawn in a submodule directory.  The callers set up a
child_process structure, find the working tree path of one submodule 
and set .dir field to it, and then use start_command() API to spawn
the subprocess like "status", "fetch", etc.

When this happens, we expect that the ".git" (either a directory or
a gitfile that points at the real location) in the current working
directory of the subprocess MUST be the repository for the submodule.

If this ".git" thing is a corrupt repository, however, because
prepare_submodule_repo_env() unsets GIT_DIR and GIT_WORK_TREE, the
subprocess will see ".git", thinks it is not a repository, and
attempt to find one by going up, likely to end up in finding the
repository of the superproject.  In some codepaths, this will cause
a command run with the "--recurse-submodules" option to recurse
forever.

By exporting GIT_DIR=.git, disable the auto-discovery logic in the
subprocess, which would instead stop it and report an error.

Not-signed-off-yet.
---
 submodule.c |  1 +
 t/t5526-fetch-submodules.sh | 29 +
 2 files changed, 30 insertions(+)

diff --git a/submodule.c b/submodule.c
index 1b5cdfb..e8258f0 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1160,4 +1160,5 @@ void prepare_submodule_repo_env(struct argv_array *out)
if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
argv_array_push(out, *var);
}
+   argv_array_push(out, "GIT_DIR=.git");
 }
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 954d0e4..b2dee30 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -485,4 +485,33 @@ test_expect_success 'fetching submodules respects parallel 
settings' '
)
 '
 
+test_expect_success 'fetching submodule into a broken repository' '
+   # Prepare src and src/sub nested in it
+   git init src &&
+   (
+   cd src &&
+   git init sub &&
+   git -C sub commit --allow-empty -m "initial in sub" &&
+   git submodule add -- ./sub sub &&
+   git commit -m "initial in top"
+   ) &&
+
+   # Clone the old-fashoned way
+   git clone src dst &&
+   git -C dst clone ../src/sub sub &&
+
+   # Make sure that old-fashoned layout is still supported
+   git -C dst status &&
+
+   # Recursive-fetch works fine
+   git -C dst fetch --recurse-submodules &&
+
+   # Break the receiving submodule
+   rm -f dst/sub/.git/HEAD &&
+
+   # Recursive-fetch must terminate
+   # NOTE: without fix this will recurse forever!
+   test_must_fail git -C dst fetch --recurse-submodules
+'
+
 test_done

Re: [PATCH] make dist: allow using an installed version of git

[Junio C Hamano]

Dennis Kaarsemaker  writes:

> b1de9de2 back in 2005 ensured that we could create a tarball with 'make
> dist' even if git wasn't installed yet. These days however, chances are
> higher that a git version is available. Add a config.mak knob to allow
> people to choose to use the installed version of git to create the
> tarball and avoid the overhead of building git-archive.

Thanks, but not interested.

We do not know what vintage of "git" happens to be installed on the
platform, but we know how "git archive" we ship with the source
ought to behave.

>
> Signed-off-by: Dennis Kaarsemaker 
> ---
>  Makefile | 14 --
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index d96ecb7..3dabb75 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -378,6 +378,9 @@ all::
>  #
>  # to say "export LESS=FRX (and LV=-c) if the environment variable
>  # LESS (and LV) is not set, respectively".
> +#
> +# Define USE_INSTALLED_GIT_ARCHIVE if you don't want to build git-archive as
> +# part of 'make dist', but are happy to rely on a git version on you $PATH
>  
>  GIT-VERSION-FILE: FORCE
>   @$(SHELL_PATH) ./GIT-VERSION-GEN
> @@ -2423,8 +2426,15 @@ quick-install-html:
>  ### Maintainer's dist rules
>  
>  GIT_TARNAME = git-$(GIT_VERSION)
> -dist: git-archive$(X) configure
> - ./git-archive --format=tar \
> +ifndef USE_INSTALLED_GIT_ARCHIVE
> + GIT_ARCHIVE = ./git-archive$(X)
> + GIT_ARCHIVE_DEP = git-archive$(X)
> +else
> + GIT_ARCHIVE = git archive
> + GIT_ARCHIVE_DEP =
> +endif
> +dist: $(GIT_ARCHIVE_DEP) configure
> + $(GIT_ARCHIVE) --format=tar \
>   --prefix=$(GIT_TARNAME)/ HEAD^{tree} > $(GIT_TARNAME).tar
>   @mkdir -p $(GIT_TARNAME)
>   @cp configure $(GIT_TARNAME)

Re: [PATCH 09/22] sequencer: completely revamp the "todo" script parsing

[Junio C Hamano]

Johannes Schindelin  writes:

>> git continue as a shorthand for `git  --continue` sounds great.
>
> Before we get ahead of ourselves:
>
> 1) this has nothing to do with the patch series at hand, and
>
> 2) if we were to introduce `git continue`, we would need to think long and
>hard about the following issues:
>
>   I) are there potentially ambiguous s that the user
>  may want to continue?
>
>   II) what about options? You can say `git rebase --continue
>   --no-ff`, for example, but not `git cherry-pick --continue
>   --no-ff`...
>
>   III) Would it not be confusing to have a subcommand `continue`
>that does *not* serve a *single* purpose? It's kinda flying
>into the face of the Unix philosophy.

The above reasoning applies equally to "git abort".  I do not think
"git continue" would help.

If it were that anything you can do with Git can be --continue'ed
the same way (e.g. all uses one sequencer to rule them all), it
might be achievable, but I do not think it isn't, and will never be.

"git commit" may say "You haven't added anything yet" and refuse to
do anything.  Should "git continue" do "git commit -a" by noticing
that the last thing you tried to do was "git commit" and guess that
it is likely you wanted to commit all changes?  I think not.

Re: [PATCH v6 13/13] read-cache: make sure file handles are not inherited by child processes

[Junio C Hamano]

Torsten Bögershausen  writes:

>> 
>> diff --git a/sha1_file.c b/sha1_file.c
>> index d5e1121..759991e 100644
>> --- a/sha1_file.c
>> +++ b/sha1_file.c
>> @@ -1485,7 +1485,7 @@ int check_sha1_signature(const unsigned char *sha1, 
>> void *map,
>>  
>>  int git_open_noatime(const char *name)
>
> Hm, should the function then be renamed into
>
> git_open_noatime_cloexec()
>
>>  {
>> -static int sha1_file_open_flag = O_NOATIME;
>> +static int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;

Perhaps.

In any case, this is probably something that can and should be done
outside this series.

I am tempted to suggest that the patch 13/13 under discussion may
also want to be done outside the scope of, and before, this series.
Even though with the current system an inherited file descriptor to
v1 filter processes would cause issues, there is no good reason to
expose this file desciptor to them.

Thanks.

Re: git submodules implementation question

[Junio C Hamano]

Stefan Beller  writes:

>> The final version needs to be accompanied with tests to show the
>> effect of this change for callers.  A test would set up a top-level
>> and submodule, deliberately break submodule/.git/ repository and
>> show what breaks and how without this change.
>
> Tests are really good at providing this context as well, or to communicate
> the actual underlying problem, which is not quite clear to me.
> That is why I refrained from jumping into the discussion as I think the
> first few emails were dropped from the mailing list and I am missing context.

I do not know where you started reading, but the gist of it is that
submodule.c spawns subprocess to run in the submodule's context by
assuming that chdir'ing into the  of the submodule and running
it (i.e. cp.dir set to  to drive start_command()) is
sufficient.  When /.git (either it is a directory itself or it
points at a directory in .git/module/ in the superproject) is
a corrupt repository, running "git -C  command" would try to
auto-detect the repository, because it thinks /.git is not a
repository and it thinks it is not at the top-level of the working
tree, and instead finds the repository of the top-level, which is
almost never what we want.

Re: `make profile-install` fails in 2.9.3

[Thomas Gummerer]

[+cc Edward Thomson, Ingo Brückl]

Hi,

On 09/01, Jan Keromnes wrote:

[.. snip the parts others are more qualified to answer ..]
> 
> Related problem: `t3700-add.sh` currently fails in 2.9.3. I can
> provide more debug information if you don't already know this problem.

I noticed this problem as well, when I'm compiling with USE_NSEC = 1
in my config.mak.

Tracking this problem down a bit, it happens because the --chmod=[+-]x
option introduced in 4e55ed32 ("add: add --chmod=+x / --chmod=-x
options") only works if the file on disk is modified.  When the test
was changed to work on one single file, instead of doing chmod=+x on
one file and chmod=-x on another file in b38ab197c ("t3700: merge two
tests into one"), this test started breaking when the mtime of the
file and the index file weren't the same (in other words, if the file
was not racily clean and thus was not smudged).

When the file is racily clean, git detects that the contents changed,
and everything is happy, but if it isn't, git incorectly thinks the
file wasn't modified (which it wasn't, but from gits view it should be
viewed as modified because the mode does not match up anymore).

One possible fix for the test is to smudge the entry as showed below,
though I'm not sure it's the right fix.  The other way I can think of
is to change the file in the index regardless of whether the file was
changed in some other way before issuing the git add command, as that
might fit the user expectation better.  Thoughts?

diff --git a/read-cache.c b/read-cache.c
index 491e52d..f2e7986 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -656,11 +656,13 @@ int add_to_index(struct index_state *istate, const char 
*path, struct stat *st,
else
ce->ce_flags |= CE_INTENT_TO_ADD;
 
-   if (S_ISREG(st_mode) && force_mode)
+   if (S_ISREG(st_mode) && force_mode) {
ce->ce_mode = create_ce_mode(force_mode);
-   else if (trust_executable_bit && has_symlinks)
+   ce->ce_stat_data.sd_size = 0;
+   } else if (trust_executable_bit && has_symlinks) {
ce->ce_mode = create_ce_mode(st_mode);
-   else {
+   } else {
/* If there is an existing entry, pick the mode bits and type
 * from it, otherwise assume unexecutable regular file.
 */

   



> Thanks,
> Jan Keromnes
> 
> ---
> 
> Steps to reproduce:
> 
> curl https://www.kernel.org/pub/software/scm/git/git-2.9.3.tar.xz | tar 
> xJ \
>  && cd git-2.9.3 \
>  && make prefix=/usr profile-install install-man -j18
> 
> Expected result:
> 
> - runs all tests to get a profile (ignoring occasional failures)
> - rebuilds Git with the profile
> - installs Git
> 
> Actual result:
> 
> - runs all tests to get a profile
> - at least one test fails, interrupting the whole process
> - Git is not installed
> 
> Failure log:
> 
> # failed 1 among 40 test(s)
> 1..40
> Makefile:43: recipe for target 't3700-add.sh' failed
> make[3]: *** [t3700-add.sh] Error 1
> make[3]: Leaving directory '/tmp/git/git-2.9.3/t'
> Makefile:36: recipe for target 'test' failed
> make[2]: Leaving directory '/tmp/git/git-2.9.3/t'
> make[2]: *** [test] Error 2
> Makefile:2221: recipe for target 'test' failed
> make[1]: *** [test] Error 2
> make[1]: Leaving directory '/tmp/git/git-2.9.3'
> Makefile:1633: recipe for target 'profile' failed
> make: *** [profile] Error 2
> The command '/bin/sh -c mkdir /tmp/git  && cd /tmp/git  && curl
> https://www.kernel.org/pub/software/scm/git/git-2.9.3.tar.xz | tar xJ
> && cd git-2.9.3  && make prefix=/usr profile-install install-man -j18
> && rm -rf /tmp/git' returned a non-zero code: 2

-- 
Thomas

Re: [PATCH 08/22] sequencer: remove overzealous assumption

[Jakub Narębski]

Hello Johannes,

W dniu 01.09.2016 o 10:01, Johannes Schindelin pisze:
> On Wed, 31 Aug 2016, Jakub Narębski wrote:
>> W dniu 31.08.2016 o 20:36, Johannes Schindelin pisze:
>>
>> I wonder: would 'git cherry-pick --continue' be able to finish
>> 'git revert', and vice versa, then?  Or 'git sequencer --continue'?
> 
> I just tested this, via
> 
>   diff --git a/t/t3510-cherry-pick-sequence.sh
>   b/t/t3510-cherry-pick-sequence.sh
>   index 96c7640..085d8bc 100755
>   --- a/t/t3510-cherry-pick-sequence.sh
>   +++ b/t/t3510-cherry-pick-sequence.sh
>   @@ -55,7 +55,7 @@ test_expect_success 'cherry-pick
>   mid-cherry-pick-sequence' '
>   git checkout HEAD foo &&
>   git cherry-pick base &&
>   git cherry-pick picked &&
>   -   git cherry-pick --continue &&
>   +   git revert --continue &&
>   git diff --exit-code anotherpick
> 
> (Danger! Whitespace corrupted!!!)
> 
> It appears that this passes now.

I'm now not sure if it is such a great idea.  As was said somewhere else
in this thread, different sequencer-based commands sports different
options, and you can add options to the "git  --continue".
For example you can say "git cherry-pick --continue -x", but you
cannot say "git revert --continue -x", as '-x' is a cherry-pick only
option.  Or you can, theoretically, use "git am --continue --no-3way".

One option is to temporarily relax the test (test_expect_failure),
then fix it at the end.


BTW. how git-am uses sequencer?  I have seen "revert" etc., and "pick"
etc., but no git-am related constants or strings...

> Probably `git sequencer --continue` would work, too, if there was a `git
> sequencer`. :0)

Right.

> 
>>> On Wed, 31 Aug 2016, Jakub Narębski wrote: 
 W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
>>  
> diff --git a/t/t3510-cherry-pick-sequence.sh 
> b/t/t3510-cherry-pick-sequence.sh
> index 7b7a89d..6465edf 100755
> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -459,17 +459,6 @@ test_expect_success 'malformed instruction sheet 1' '
>   test_expect_code 128 git cherry-pick --continue
>  '
>  
> -test_expect_success 'malformed instruction sheet 2' '

 Hmmm... the description is somewhat lacking (especially compared to
 the rest of test), anyway.

 BTW. we should probably rename 'malformed instruction sheet 2'
 to 'malformed instruction sheet' if there are no further such
 tests after this removal, isn't it?
>>>
>>> No, we cannot rename it after this patch because the patch removes it ;-)
>>> (It is not a file name but really a label for a test case.)
>>
>> Ooops.  What I wanted to say that after removing the test case named
>> 'malformed instruction sheet 2' we should also rename *earlier* test
>> case from 'malformed instruction sheet 1' to 'malformed instruction sheet',
>> as it is now the only 'malformed instruction sheet *' test case.
> 
> Actually, you know, I completely missed the fact that there was a
> "malformed instruction sheet 3". I renumbered it.

Ooops.  I have missed it too, having looked only at the test after the
one removed (which is not about malformed instruction sheet).

Best,
-- 
Jakub Narębski

Re: Are --first-parent and --ancestry-path compatible rev-list options?

[Philip Oakley]

Hi Junio,

From: "Junio C Hamano" 

"Philip Oakley"  writes:


From: "Junio C Hamano" 

"Philip Oakley"  writes:


The commit graph. We are looking for F based on knowing J.

. A - B - C - D -- E -- F -- G - H<-first parent, --merges (C,F,H)
.  \  |  /\//
.   Z |   //
. |   |   |   /
.  \   \ /   /
.   I -[J]- K - L - M <-since J, children of J
.\ /
. N - O - P


I think these two operations are fundamentally incompatible.


If I run them independently, they both find the desired INTERESTED
commit, hence the expectation that together they will still find that
commit as an intersection between the two sets.



Because the first-parent traversal is what the name says, i.e.,
forbids the positive side of revision traversal to stray into side
branches, the positive side of a traversal that begins at H will not
see M, L and K.


But it does see F the ultimately desired commit.


You are doing --merges --first-parent, right?  Traversing only the
first-parent chain on the positive side, while excluding J's
ancestor by traversing the negative side without being limited to
the first-parent chain, would paint B and its ancestors as
uninteresting on the first-parent chain, so among H, G, F, E, D and
C, which are the survivors on the first-parent chain that are still
not UNINTERESTIN, the last one you would find that is a merge is F.

So I do not see any room for "But" to come in here...



The confusion is between the "As required" and "as coded" viewpoints (this 
is regular dayjob problem of allegedly having 'requirement specifications').


You have rightly described the algorithm as currently implemented, while I 
was was trying to state a requirement (based on a user question).


The user question was, given a commit 'J', and a future commit 'H' 
(typically a branch tip such as 'master'), find those commits that are :

A) merges
B) on the first parent DAG chain of the future commit 'H'
C) children of the given commit 'J'

i.e. the points on master where the feature J (and it's children) could have 
brought in some effect to master.


Each of the three conditions match one of the revision list options, but the 
implemented logic does not produce the AND effect that could be expected. 
Essentially it (the user's desire vs the coding implementation) is a case 
where (depending on the viewpoint) we get the 'denying the antecedent' 
fallacy.


Just because a commit is not --first-parent does not mean that revison 
walking (in this case) should stop. The user is interested in the ancestry 
path, so from that perspective the the walk should carry on through the 
other parents till its reaches the TRULY_DISINTERESTED line, or reaches J.



With the current implementation one appears to need to script a search 
through all the first parent merges and then prune out those that aren't 
merge-base is-ancestor commits, which is what the OP ended up having to do.



I haven't had any time to look into the rev-walk code itself, but is this 
something that could be coded (new option) or is the double-duty problem 
with UNINTERESTING too hard baked into the code?

--
Philip

Re: [PATCH 07/34] sequencer (rebase -i): add support for the 'fixup' and 'squash' commands

[Junio C Hamano]

Dennis Kaarsemaker  writes:

>> +static const char *nth_for_number(int n)
>> +{
>> +int n1 = n % 10, n10 = n % 100;
>> +
>> +if (n1 == 1 && n10 != 11)
>> +return "st";
>> +if (n1 == 2 && n10 != 12)
>> +return "nd";
>> +if (n1 == 3 && n10 != 13)
>> +return "rd";
>> +return "th";
>> +}
>
>>8---
>
>> +if (command == TODO_SQUASH) {
>> +unlink(rebase_path_fixup_msg());
>> +strbuf_addf(, "\n%c This is the %d%s commit message:\n\n%s",
>> +comment_line_char,
>> +count, nth_for_number(count), body);
>> +}
>> +else if (command == TODO_FIXUP) {
>> +strbuf_addf(,
>> +"\n%c The %d%s commit message will be skipped:\n\n",
>> +comment_line_char, count, nth_for_number(count));
>> +strbuf_add_commented_lines(, body, strlen(body));
>> +}
>
> This way of handling numbers is not translatable, and I really think we
> should mark these strings for translation, like they are in the .sh
> version.

Correct.

For those who were not paying attention on the 'master' front during
this pre-release period [*1*], I have to point out that the scripted
Porcelain has been updated to lose the Anglo-centric st/nd/rd/th and
this series would want to get updated to match.


[Footnote]

*1* Why weren't you?  Repent! ;-)

Re: [PATCH 21/22] sequencer: left-trim the lines read from the script

[Junio C Hamano]

Johannes Schindelin  writes:

>> > Interactive rebase's scripts may be indented; We need to handle this
>> > case, too, now that we prepare the sequencer to process interactive
>> > rebases.
>> 
>> s/; We need/; we need/
>
> Hrmpf. From http://grammar.ccc.commnet.edu/grammar/marks/colon.htm:
>
>   There is some disagreement among writing reference manuals about
>   when you should capitalize an independent clause following a
>   colon. Most of the manuals advise that when you have more than one
>   sentence in your explanation or when your sentence(s) is a formal
>   quotation, a capital is a good idea. The NYPL Writer's Guide urges
>   consistency within a document; the Chicago Manual of Style says
>   you may begin an independent clause with a lowercase letter unless it's
>   one of those two things (a quotation or more than one sentence).
>   The APA Publication Manual is the most extreme: it advises us to
>   always capitalize an independent clause following a colon. The advice
>   given above is consistent with the Gregg Reference Manual.
>
> Based on that, I think that a capital is the correct case here.

Does that manual have anything to say about semicolons, which is a
different thing?

Re: `make profile-install` fails in 2.9.3

[Dennis Kaarsemaker]

On do, 2016-09-01 at 18:08 +0200, Jan Keromnes wrote:

> However, this fails (and has failed in previous versions), because it
> runs the whole test-suite to get the profile, but bails out if there
> were test failures (which happens often).

Working around failing tests is fixing a symptom, not the root cause. I
run the testsuite for master, next and pu very regularly and test
failures on master are extremely rare. I think I've seen one or so in
the last year, might even be 0. So let's focus on that instead.

> Related problem: `t3700-add.sh` currently fails in 2.9.3. I can
> provide more debug information if you don't already know this problem.

It should not fail, and for me does not fail on ubuntu 16.04. Please
run that test in verbose mode and share its output, as well as your
build configuration.

D.

Re: [PATCH 07/34] sequencer (rebase -i): add support for the 'fixup' and 'squash' commands

[Dennis Kaarsemaker]

On do, 2016-09-01 at 17:17 +0200, Johannes Schindelin wrote:
> Hi Dennis,
> 
> On Thu, 1 Sep 2016, Dennis Kaarsemaker wrote:
> 
> > 
> > On wo, 2016-08-31 at 10:54 +0200, Johannes Schindelin wrote:
> > 
> > > 
> > > +static int is_fixup(enum todo_command command)
> > > +{
> > > + return command == TODO_FIXUP || command == TODO_SQUASH;
> > > +}
> > It sounds wrong to have a function named is_fixup return true when
> > the
> > command isn't a fixup but a squash. Maybe name it
> > changes_previous_commit or something?
> I can see how that may sound confusing, unless you understand that a
> squash is a fixup that lets the user edit the commit message, too. So
> essentially squash = fixup + edit, if you will.
> 
> Maybe the name is more appropriate in that light?

Kinda makes sense. It's not how I use fixup/squash as a user of rebase
-i though. But we can't go there, that's bikeshed country :)

> > > +static const char *nth_for_number(int n)
> > > +{
> > > + int n1 = n % 10, n10 = n % 100;
> > > +
> > > + if (n1 == 1 && n10 != 11)
> > > + return "st";
> > > + if (n1 == 2 && n10 != 12)
> > > + return "nd";
> > > + if (n1 == 3 && n10 != 13)
> > > + return "rd";
> > > + return "th";
> > > +}
> > > 
> > > 8---
> > > 
> > > + if (command == TODO_SQUASH) {
> > > + unlink(rebase_path_fixup_msg());
> > > + strbuf_addf(, "\n%c This is the %d%s commit
> > > message:\n\n%s",
> > > + comment_line_char,
> > > + count, nth_for_number(count), body);
> > > + }
> > > + else if (command == TODO_FIXUP) {
> > > + strbuf_addf(,
> > > + "\n%c The %d%s commit message will be
> > > skipped:\n\n",
> > > + comment_line_char, count,
> > > nth_for_number(count));
> > > + strbuf_add_commented_lines(, body,
> > > strlen(body));
> > > + }
> > This way of handling numbers is not translatable, and I really
> > think we
> > should mark these strings for translation, like they are in the .sh
> > version.
> Ah, this is the risk of working on something as big as rebase
> --helper.
> Back when I started with it, the relevant code in git-rebase
> --interactive
> read like this:
> 
>   nth_string () {
>   case "$1" in
>   *1[0-9]|*[04-9]) echo "$1"th;;
>   *1) echo "$1"st;;
>   *2) echo "$1"nd;;
>   *3) echo "$1"rd;;
>   esac
>   }
> 
> I merely did a faithful translation of that...
> 
> Now, I see that git-rebase--interactive was switched to use
> eval_gettext,
> which in turn is handled in git-sh-i18n whose code is quite
> convoluted. In
> the absence of gettext, it uses git-sh-i18n--envsubst, which has no C
> API
> whatsoever.
> 
> And I see that the beautiful ordinal computation was given up in
> favor of
> a lousy "#1", "#2", "#3", etc (it used to be "1st", "2nd", "3rd"
> etc).
> 
> In any case, translation is not my main concern until v2.10.0, so
> I'll
> take care of this after that release.

Hmm, not sure if I agree with that. I'd see it as a regression to lose
the i18n there.

D.

Re: [PATCH 27/34] sequencer (rebase -i): differentiate between comments and 'noop'

[Dennis Kaarsemaker]

On do, 2016-09-01 at 17:32 +0200, Johannes Schindelin wrote:
> Hi Dennis,
> 
> On Thu, 1 Sep 2016, Dennis Kaarsemaker wrote:
> 
> > 
> > On wo, 2016-08-31 at 10:56 +0200, Johannes Schindelin wrote:
> > > 
> > > diff --git a/sequencer.c b/sequencer.c
> > > index 51c2f76..4c902e5 100644
> > > --- a/sequencer.c
> > > +++ b/sequencer.c
> > > @@ -763,7 +763,8 @@ enum todo_command {
> > > TODO_SQUASH,
> > > TODO_EXEC,
> > > TODO_NOOP,
> > > -   TODO_DROP
> > > +   TODO_DROP,
> > > +   TODO_COMMENT
> > >  };
> > (picking a random commit that touches this enum)
> > 
> > In a few places you now make comparisons like "< TODO_NOOP", so I
> > think
> > it would be good to have a comment near the definition of this enum
> > that says that ordering matters and why, so people don't attempt to
> > add
> > a new TODO_FOOBAR at the end.
> True.
> 
> It does not seem that we have a precedent for that. The closest is
> what I
> had in an early iteration of the fsck message IDs, and subsequently
> things
> were refactored so that it is not the order, but a flag, that
> determines
> what the command does.
> 
> Not sure how to do this elegantly. Maybe like this?
> 
>   enum todo_command {
>   TODO_PICK_COMMANDS = 0,
>   TODO_PICK = TODO_PICK_COMMANDS,
>   TODO_SQUASH,
> 
>   TODO_NON_PICK_COMMANDS,
>   TODO_EXEC = TODO_NON_PICK_COMMANDS,
> 
>   TODO_NOOP_COMMANDS,
>   TODO_NOOP = TODO_NOOP_COMMANDS,
>   TODO_DROP
>   TODO_DROP,
> 
>   TODO_LAST_COMMAND,
>   TODO_COMMENT = TODO_LAST_COMMAND
>   };
> 
> But that is so god-awful to read.

Agreed, that sure is awful.

How about something like

/*
 * Note that ordering matters in this enum. Not only must it match the
 * mapping below, it is also divided into several sections that matter.
 * When adding new commands, make sure you add it in the right section.
 */
enum todo_command {
/* All commands that handle commits */
TODO_PICK,
...
/* All commands that do something else than pick */
TODO_EXEC,
...
/* All commands that do nothing but are counted for reporting progress 
*/
TODO_NOOP,
...
/* Comments, which are not counted
TODO_COMMENT
}

`make profile-install` fails in 2.9.3

[Jan Keromnes]

Hello,

I'm trying to `profile-install` Git from source on Ubuntu 16.04, to
have the latest stable Git optimized for my machine.

However, this fails (and has failed in previous versions), because it
runs the whole test-suite to get the profile, but bails out if there
were test failures (which happens often).

Problem: Is there a way to `make profile-install` but ignore
occasional test failures, as they're not problematic to get a useful
profile?

Related problem: `t3700-add.sh` currently fails in 2.9.3. I can
provide more debug information if you don't already know this problem.

Thanks,
Jan Keromnes

---

Steps to reproduce:

curl https://www.kernel.org/pub/software/scm/git/git-2.9.3.tar.xz | tar xJ \
 && cd git-2.9.3 \
 && make prefix=/usr profile-install install-man -j18

Expected result:

- runs all tests to get a profile (ignoring occasional failures)
- rebuilds Git with the profile
- installs Git

Actual result:

- runs all tests to get a profile
- at least one test fails, interrupting the whole process
- Git is not installed

Failure log:

# failed 1 among 40 test(s)
1..40
Makefile:43: recipe for target 't3700-add.sh' failed
make[3]: *** [t3700-add.sh] Error 1
make[3]: Leaving directory '/tmp/git/git-2.9.3/t'
Makefile:36: recipe for target 'test' failed
make[2]: Leaving directory '/tmp/git/git-2.9.3/t'
make[2]: *** [test] Error 2
Makefile:2221: recipe for target 'test' failed
make[1]: *** [test] Error 2
make[1]: Leaving directory '/tmp/git/git-2.9.3'
Makefile:1633: recipe for target 'profile' failed
make: *** [profile] Error 2
The command '/bin/sh -c mkdir /tmp/git  && cd /tmp/git  && curl
https://www.kernel.org/pub/software/scm/git/git-2.9.3.tar.xz | tar xJ
&& cd git-2.9.3  && make prefix=/usr profile-install install-man -j18
&& rm -rf /tmp/git' returned a non-zero code: 2

Re: git submodules implementation question

[Uma Srinivasan]

Yes, this one line fix addresses my problem.

So, what is the next step? Will someone submit a patch or should I?
Please note that I've never submitted a patch before, but I don't mind
learning how to.

Thanks,
Uma

> --- a/submodule.c
> +++ b/submodule.c
> @@ -1160,4 +1160,5 @@ void prepare_submodule_repo_env(struct argv_array *out)
> if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
> argv_array_push(out, *var);
> }
> +   argv_array_push(out, "GIT_DIR=.git");
>  }
>

Re: Notation for current branch?

[ryenus]

> Jacob Keller  writes:
> "git symbolic-ref" seems like the right thing if you need to obtain
> the current branch name, and there's no reason to not just use HEAD
> there.

Really? Any reason why `git rev-parse --abbrev-ref '@{-1}'` works,
but not `git symbolic-ref '@{-1}'`, or even `git symbolic-ref @`?

BTW, possible to make the curly brances optional in `@{n}`, `@{-n}`,
and the alike? coz they require quoting/escaping, and more typing :(

Re: [PATCH 27/34] sequencer (rebase -i): differentiate between comments and 'noop'

[Johannes Schindelin]

Hi Dennis,

On Thu, 1 Sep 2016, Dennis Kaarsemaker wrote:

> On wo, 2016-08-31 at 10:56 +0200, Johannes Schindelin wrote:
> > diff --git a/sequencer.c b/sequencer.c
> > index 51c2f76..4c902e5 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -763,7 +763,8 @@ enum todo_command {
> > TODO_SQUASH,
> > TODO_EXEC,
> > TODO_NOOP,
> > -   TODO_DROP
> > +   TODO_DROP,
> > +   TODO_COMMENT
> >  };
> 
> (picking a random commit that touches this enum)
> 
> In a few places you now make comparisons like "< TODO_NOOP", so I think
> it would be good to have a comment near the definition of this enum
> that says that ordering matters and why, so people don't attempt to add
> a new TODO_FOOBAR at the end.

True.

It does not seem that we have a precedent for that. The closest is what I
had in an early iteration of the fsck message IDs, and subsequently things
were refactored so that it is not the order, but a flag, that determines
what the command does.

Not sure how to do this elegantly. Maybe like this?

enum todo_command {
TODO_PICK_COMMANDS = 0,
TODO_PICK = TODO_PICK_COMMANDS,
TODO_SQUASH,

TODO_NON_PICK_COMMANDS,
TODO_EXEC = TODO_NON_PICK_COMMANDS,

TODO_NOOP_COMMANDS,
TODO_NOOP = TODO_NOOP_COMMANDS,
TODO_DROP
TODO_DROP,

TODO_LAST_COMMAND,
TODO_COMMENT = TODO_LAST_COMMAND
};

But that is so god-awful to read.

Still unsure,
Dscho

format-patch base-commit: moving to above the patch?

[Josh Triplett]

Currently, format-patch puts base-commit and prerequisite-patch-id
information below the patch, and below the email signature.  Most mail
clients automatically trim everything below the signature marker as
unimportant when quoting a mail for a reply, which would make it
difficult for someone to reply, quote the base-commit, and say something
like "I don't have this commit, where did it come from?" or "Can you
please rebase this on ...".

Might it make sense to move this information adjacent to the diffstat,
instead?  Or, at least, above the email signature?

Re: [PATCH 32/34] sequencer (rebase -i): show the progress

[Johannes Schindelin]

Hi Dennis,

On Thu, 1 Sep 2016, Dennis Kaarsemaker wrote:

> On wo, 2016-08-31 at 10:56 +0200, Johannes Schindelin wrote:
> > The interactive rebase keeps the user informed about its progress.
> > If the sequencer wants to do the grunt work of the interactive
> > rebase, it also needs to show that progress.
> > 
> > Signed-off-by: Johannes Schindelin 
> > ---
> >  sequencer.c | 33 +
> >  1 file changed, 33 insertions(+)
> > 
> > diff --git a/sequencer.c b/sequencer.c
> > index 89fd625..e8c6daf 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -1218,6 +1218,7 @@ struct todo_list {
> >     struct strbuf buf;
> >     struct todo_item *items;
> >     int nr, alloc, current;
> > +   int done_nr, total_nr;
> >  };
> >  
> >  #define TODO_LIST_INIT { STRBUF_INIT }
> > @@ -1329,6 +1330,17 @@ static int parse_insn_buffer(char *buf, struct 
> > todo_list *todo_list)
> >     return res;
> >  }
> >  
> > +static int count_commands(struct todo_list *todo_list)
> > +{
> > +   int count = 0, i;
> > +
> > +   for (i = 0; i < todo_list->nr; i++)
> > +   if (todo_list->items[i].command != TODO_COMMENT)
> > +   count++;
> > +
> > +   return count;
> > +}
> > +
> >  static int read_populate_todo(struct todo_list *todo_list,
> >     struct replay_opts *opts)
> >  {
> > @@ -1355,6 +1367,22 @@ static int read_populate_todo(struct todo_list 
> > *todo_list,
> >     if (!todo_list->nr &&
> >     (!is_rebase_i(opts) || !file_exists(rebase_path_done(
> >     return error(_("No commits parsed."));
> > +
> > +   if (is_rebase_i(opts)) {
> > +   struct todo_list done = TODO_LIST_INIT;
> > +
> > +   if (strbuf_read_file(, rebase_path_done(), 0) > 0 &&
> > +   !parse_insn_buffer(done.buf.buf, ))
> > +   todo_list->done_nr = count_commands();
> > +   else
> > +   todo_list->done_nr = 0;
> > +
> > +   todo_list->total_nr = todo_list->done_nr
> > +   + count_commands(todo_list);
> > +
> > +   todo_list_release();
> > +   }
> > +
> >     return 0;
> >  }
> >  
> > @@ -1900,6 +1928,11 @@ static int pick_commits(struct todo_list *todo_list, 
> > struct replay_opts *opts)
> >     if (save_todo(todo_list, opts))
> >     return -1;
> >     if (is_rebase_i(opts)) {
> > +   if (item->command != TODO_COMMENT)
> > +   fprintf(stderr, "Rebasing (%d/%d)%s",
> > +   ++(todo_list->done_nr),
> > +   todo_list->total_nr,
> > +   opts->verbose ? "\n" : "\r");
> >     unlink(rebase_path_message());
> >     unlink(rebase_path_author_script());
> >     unlink(rebase_path_stopped_sha());
> 
> (picking a random commit that shows this 'symptom')
> 
> You're sprinking a lot of is_rebase_i's around sequencer.c to make sure
> there are no changes in behaviour. I wonder if the right balance has
> been struck between 'no changes in behaviour' and 'common behaviour'.
> For instance, in this case, maybe it would be a better idea for non-
> rebase uses of the sequencer to also show progress.

Actually, adding progress would make for a fine add-on patch series. I
still would like to have as faithful a conversion as possible, and
everything else can come after that.

This strategy has a couple of advantages:

- we can concentrate on correctness for now,

- I get something to show for my time with Git for Windows v2.10.0, and

- the add-on patches do not have to be done by *me* ;-)

Ciao,
Dscho

Re: [PATCH 07/34] sequencer (rebase -i): add support for the 'fixup' and 'squash' commands

[Johannes Schindelin]

Hi Dennis,

On Thu, 1 Sep 2016, Dennis Kaarsemaker wrote:

> On wo, 2016-08-31 at 10:54 +0200, Johannes Schindelin wrote:
> 
> > +static int is_fixup(enum todo_command command)
> > +{
> > +   return command == TODO_FIXUP || command == TODO_SQUASH;
> > +}
> 
> It sounds wrong to have a function named is_fixup return true when the
> command isn't a fixup but a squash. Maybe name it
> changes_previous_commit or something?

I can see how that may sound confusing, unless you understand that a
squash is a fixup that lets the user edit the commit message, too. So
essentially squash = fixup + edit, if you will.

Maybe the name is more appropriate in that light?

> > +static const char *nth_for_number(int n)
> > +{
> > +   int n1 = n % 10, n10 = n % 100;
> > +
> > +   if (n1 == 1 && n10 != 11)
> > +   return "st";
> > +   if (n1 == 2 && n10 != 12)
> > +   return "nd";
> > +   if (n1 == 3 && n10 != 13)
> > +   return "rd";
> > +   return "th";
> > +}
> 
> >8---
> 
> > +   if (command == TODO_SQUASH) {
> > +   unlink(rebase_path_fixup_msg());
> > +   strbuf_addf(, "\n%c This is the %d%s commit message:\n\n%s",
> > +   comment_line_char,
> > +   count, nth_for_number(count), body);
> > +   }
> > +   else if (command == TODO_FIXUP) {
> > +   strbuf_addf(,
> > +   "\n%c The %d%s commit message will be skipped:\n\n",
> > +   comment_line_char, count, nth_for_number(count));
> > +   strbuf_add_commented_lines(, body, strlen(body));
> > +   }
> 
> This way of handling numbers is not translatable, and I really think we
> should mark these strings for translation, like they are in the .sh
> version.

Ah, this is the risk of working on something as big as rebase--helper.
Back when I started with it, the relevant code in git-rebase--interactive
read like this:

nth_string () {
case "$1" in
*1[0-9]|*[04-9]) echo "$1"th;;
*1) echo "$1"st;;
*2) echo "$1"nd;;
*3) echo "$1"rd;;
esac
}

I merely did a faithful translation of that...

Now, I see that git-rebase--interactive was switched to use eval_gettext,
which in turn is handled in git-sh-i18n whose code is quite convoluted. In
the absence of gettext, it uses git-sh-i18n--envsubst, which has no C API
whatsoever.

And I see that the beautiful ordinal computation was given up in favor of
a lousy "#1", "#2", "#3", etc (it used to be "1st", "2nd", "3rd" etc).

In any case, translation is not my main concern until v2.10.0, so I'll
take care of this after that release.

Ciao,
Dscho

Re: [PATCH 20/34] sequencer (rebase -i): copy commit notes at end

[Johannes Schindelin]

Hi Dennis,

On Thu, 1 Sep 2016, Dennis Kaarsemaker wrote:

> On wo, 2016-08-31 at 10:55 +0200, Johannes Schindelin wrote:
> > +   if (!stat(rebase_path_rewritten_list(), ) &&
> > +   st.st_size > 0) {
> > +   struct child_process child = CHILD_PROCESS_INIT;
> > +
> > +   child.in = open(rebase_path_rewritten_list(), 
> > O_RDONLY);
> > +   child.git_cmd = 1;
> > +   argv_array_push(, "notes");
> > +   argv_array_push(, "copy");
> > +   argv_array_push(, 
> > "--for-rewrite=rebase");
> > +   /* we don't care if this copying failed */
> > +   run_command();
> > +   }
> 
> I know this is a strict port of git-rebase--interactive.sh, but
> shouldn't we at least warn the user that the copy failed?

At this point, I want to have as faithful a conversion as possible (except
that I do not care for the speed of git-rebase--interactive.sh, of
course).

Besides, I am fairly certain that a failure will result in an error
message. We just do not act on the exit value.

Ciao,
Dscho

Re: implement a stable 'Last updated' in Documentation

[Olaf Hering]

Hey, asciidoc made a move, so this patch is good to go:
https://github.com/asciidoc/asciidoc/pull/9

Thanks.

Olaf

On Tue, Feb 10, Jeff King wrote:

> On Tue, Feb 10, 2015 at 04:17:47PM +0100, Olaf Hering wrote:
> 
> > On Fri, Jan 30, Jeff King wrote:
> > 
> > > I have 8.6.9-3 installed (it is part of Debian testing/unstable now),
> > > and confirmed that:
> > > 
> > > diff --git a/Documentation/asciidoc.conf b/Documentation/asciidoc.conf
> > > index 2c16c53..10c777e 100644
> > > --- a/Documentation/asciidoc.conf
> > > +++ b/Documentation/asciidoc.conf
> > > @@ -21,6 +21,7 @@ tilde=
> > >  apostrophe=
> > >  backtick=
> > >  litdd=
> > > +footer-style=none
> > >  
> > >  ifdef::backend-docbook[]
> > >  [linkgit-inlinemacro]
> > > 
> > > drops the "last-updated" footer.
> > 
> > This does not remove "Last updated" from files like
> > using-merge-subtree.html for me, using asciidoc-8.6.8.
> 
> Yes, the feature is part of 8.6.9-3.
> 
> -Peff

Re: [PATCH 21/22] sequencer: left-trim the lines read from the script

[Johannes Schindelin]

Hi Kuba,

On Thu, 1 Sep 2016, Jakub Narębski wrote:

> W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:
> 
> Subject: [PATCH 21/22] sequencer: left-trim the lines read from the script
> 
> In the subject, it should probably be without "the", as "lines"
> are plural.
> 
> s/left-trim the lines/left-trim lines/

I am happy that we stepped outside of the "code correctness" land into
"grammar fix" land, as it surely means that you are convinced the code is
correct? ;-)

Fixed.

> > Interactive rebase's scripts may be indented; We need to handle this
> > case, too, now that we prepare the sequencer to process interactive
> > rebases.
> 
> s/; We need/; we need/

Hrmpf. From http://grammar.ccc.commnet.edu/grammar/marks/colon.htm:

There is some disagreement among writing reference manuals about
when you should capitalize an independent clause following a
colon. Most of the manuals advise that when you have more than one
sentence in your explanation or when your sentence(s) is a formal
quotation, a capital is a good idea. The NYPL Writer's Guide urges
consistency within a document; the Chicago Manual of Style says
you may begin an independent clause with a lowercase letter unless it's
one of those two things (a quotation or more than one sentence).
The APA Publication Manual is the most extreme: it advises us to
always capitalize an independent clause following a colon. The advice
given above is consistent with the Gregg Reference Manual.

Based on that, I think that a capital is the correct case here.

> 'bol' is beginning-of-line, isn't it (a complement to eol)?

Yep. How did you guess? :-)

Ciao,
Dscho

Re: [PATCH 22/22] sequencer: refactor write_message()

[Johannes Schindelin]

Hi Kuba,

On Thu, 1 Sep 2016, Jakub Narębski wrote:

> W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:
> 
> > The write_message() function safely writes an strbuf to a file.
> > Sometimes this is inconvenient, though: the text to be written may not
> > be stored in a strbuf, or the strbuf should not be released after
> > writing.
> 
> By "this" you mean "using strbuf", isn't it?  It is not very obvious,
> and I think it would be better to say it explicitly.

Rephrased.

> > Let's allow for such use cases by refactoring write_message() to allow
> > for a convenience function write_file_gently(). As some of the
> > upcoming callers of that new function will want to append a newline
> > character, let's just add a flag for that, too.
> 
> This paragraph feels a bit convoluted.
> 
> As I understand it, you refactor "safely writing string to a file"
> into write_with_lock_file(), and make write_message() use it.  The
> new function makes it easy to create new convenience function 
> write_file_gently(); as some of the upcoming callers of this new
> function would want to append a newline character, add a flag for
> it in write_file_gently(), and thus in write_with_lock_file().
> 
> Isn't it better / easier to understand?

I don't know, but I took it.

> > diff --git a/sequencer.c b/sequencer.c
> > index 5efed2e..f5b5e5e 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -239,22 +239,37 @@ static void print_advice(int show_hint, struct 
> > replay_opts *opts)
> > }
> >  }
> >  
> > -static int write_message(struct strbuf *msgbuf, const char *filename)
> > +static int write_with_lock_file(const char *filename,
> > +   const void *buf, size_t len, int append_eol)
> >  {
> > static struct lock_file msg_file;
> >  
> > int msg_fd = hold_lock_file_for_update(_file, filename, 0);
> > if (msg_fd < 0)
> > return error_errno(_("Could not lock '%s'"), filename);
> > -   if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
> > +   if (write_in_full(msg_fd, buf, len) < 0)
> > return error_errno(_("Could not write to %s"), filename);
> 
> You could have, for consistency, add quotes around filename (see previous
> error_errno callsite), *while at it*:
> 
>   return error_errno(_("Could not write to '%s'"), filename);

Done.

> > -   strbuf_release(msgbuf);
> > +   if (append_eol && write(msg_fd, "\n", 1) < 0)
> > +   return error_errno(_("Could not write eol to %s"), filename);
> 
> Same here, and it wouldn't even be 'while at it'

Done.

>   +   return error_errno(_("Could not write eol to '%s'"), filename);
> 
> 
> > if (commit_lock_file(_file) < 0)
> > return error(_("Error wrapping up %s."), filename);
> 
> Another "while at it"... though the one that can be safely postponed
> (well, the make message easier to understand part, not the quote
> filename part):
> 
>   return error(_("Error wrapping up writing to '%s'."), filename);

As I inherited this message, I'll keep it.

> And thus we got to the last patch in this series.  I have skipped
> patches that already got reviewed; are there some that you would
> like to have second review of?  Is there patch series that needs
> to be applied earlier that needs a review?

Thank you for your review!
Dscho

Re: [PATCH 19/22] sequencer: support cleaning up commit messages

[Johannes Schindelin]

Hi Kuba,

On Thu, 1 Sep 2016, Jakub Narębski wrote:

> W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:
> 
> > @@ -781,7 +785,7 @@ static int do_pick_commit(enum todo_command command, 
> > struct commit *commit,
> > }
> > if (!opts->no_commit)
> > res = sequencer_commit(opts->edit ? NULL : git_path_merge_msg(),
> > -   opts, allow, opts->edit, 0);
> > +   opts, allow, opts->edit, 0, 0);
> 
> The calling convention begins to look unwieldy, but we have only
> a single such callsite, and there are quite a bit callsites in
> Git code that have similar API ("git grep ', 0, 0' -- '*.c'").
> So we don't need to think about alternatives.  Yet.

Right.

Please note that it will make much more sense in the end, too, as the 0s
will be replaced by appropriate variables.

> It's a pity that emulation of named parameters in C requires
> relying on designated inits from C99
> 
>   typedef struct {
> double pressure, moles, temp;
>   } ideal_struct;
> 
>   #define ideal_pressure(...) ideal_pressure_base((ideal_struct){.pressure=1, 
>   \
> .moles=1, .temp=273.15, __VA_ARGS__})
> 
>   double ideal_pressure_base(ideal_struct in)
>   {
> return 8.314 * in.moles*in.temp/in.pressure;
>   }
> 
>   ... ideal_pressure(.moles=2, .temp=373.15) ...

Yeah, that looks unwieldy ;-)

Thanks for the review,
Dscho

Re: [PATCH 18/22] sequencer: support amending commits

[Johannes Schindelin]

Hi Kuba,

On Wed, 31 Aug 2016, Jakub Narębski wrote:

> W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:
> 
> > diff --git a/sequencer.c b/sequencer.c
> > index 7e17d14..20f7590 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -478,7 +478,7 @@ static char **read_author_script(void)
> >   * (except, of course, while running an interactive rebase).
> >   */
> >  int sequencer_commit(const char *defmsg, struct replay_opts *opts,
> > - int allow_empty, int edit)
> > + int allow_empty, int edit, int amend)
> 
> I guess we won't get much more parameters; it would get unwieldy
> (and hard to remember).  Five is all right.

It will eventually get a sixth, cleanup_commit_message.

> >  {
> > char **env = NULL;
> > struct argv_array array;
> > @@ -507,6 +507,8 @@ int sequencer_commit(const char *defmsg, struct 
> > replay_opts *opts,
> > argv_array_push(, "commit");
> > argv_array_push(, "-n");
> >  
> > +   if (amend)
> > +   argv_array_push(, "--amend");
> > if (opts->gpg_sign)
> > argv_array_pushf(, "-S%s", opts->gpg_sign);
> > if (opts->signoff)
> > @@ -779,7 +781,7 @@ static int do_pick_commit(enum todo_command command, 
> > struct commit *commit,
> > }
> > if (!opts->no_commit)
> > res = sequencer_commit(opts->edit ? NULL : git_path_merge_msg(),
> > -   opts, allow, opts->edit);
> > +   opts, allow, opts->edit, 0);
> 
> ... even of this makes one need to check the calling convention,
> what does this 0 mean.

Yeah, but it will not remain "0", but be replaced by a variable named
"amend"...

Thanks for the review,
Dscho

Re: [PATCH 16/22] sequencer: prepare for rebase -i's GPG settings

[Johannes Schindelin]

Hi Junio,

On Wed, 31 Aug 2016, Junio C Hamano wrote:

> Jakub Narębski  writes:
> 
> >> +  else {
> >> +  opts->gpg_sign = buf.buf + 2;
> >> +  strbuf_detach(, NULL);
> >
> > Wouldn't we leak 2 characters that got skipped?  Maybe xstrdup would
> > be better (if it is leaked, and not reattached)?
> 
> An attempt to avoid leaking by calling free(opts->gpg_sign) would
> make it crash, which would be even worse ;-).

As I pointed out in a couple of replies yesterday: we cannot assume that
gpg_sign is free()able. That's the entire reason behind the
sequencer_entrust() dance.

Ciao,
Dscho

Re: [PATCH 17/22] sequencer: allow editing the commit message on a case-by-case basis

[Johannes Schindelin]

Hi Kuba,

On Wed, 31 Aug 2016, Jakub Narębski wrote:

> W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:
> 
> > In the upcoming commits, we will implement more and more of rebase
> > -i's functionality. One particular feature of the commands to come is
> > that some of them allow editing the commit message while others don't,
> > i.e. we cannot define in the replay_opts whether the commit message
> > should be edited or not.
> 
> It's a nice, pretty and self contained refactoring step.  Small
> enough that it is easy to review.
> 
> I would like to have in the commit message that it is sequencer_commit()
> function that needs to rely on new parameter, instead of on a property
> of command (of its replay_opts).  And that currently it simply passes
> the buck to caller, which uses opts->edit, but in the future the
> caller that is rebase -i would use todo_item and replay_opts based
> expression.

I enhanced the commit message.

Thanks for the review,
Dscho

Re: [PATCH 16/22] sequencer: prepare for rebase -i's GPG settings

[Johannes Schindelin]

Hi Kuba,

On Wed, 31 Aug 2016, Jakub Narębski wrote:

> W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:
> 
> > The rebase command sports a `--gpg-sign` option that is heeded by the
> > interactive rebase.
> 
> Should it be "sports" or "supports"?

Funny. I got a PR last week that wanted to fix a similar expression.

I really meant "to sport", as in "To display; to have as a notable
feature.". See https://en.wiktionary.org/wiki/sport#Verb

> > +static GIT_PATH_FUNC(rebase_path_gpg_sign_opt, "rebase-merge/gpg_sign_opt")
> 
> I know it is not your fault, but I wonder why this file uses
> snake_case_name, while all other use kebab-case-names.  That is,
> why it is gpg_sign_opt and not gpg-sign-opt.

Yes, you are correct: it is not my fault ;-)

> Sidenote: it's a pity api-quote.txt is just a placeholder for proper
> documentation (including sq_quotef()).  I also wonder why it is not
> named sq_quotef_buf() or strbuf_addf_sq().

Heh. I did not even bother to check the documentation, it is my long-time
habit to dive right into the code.

> > @@ -471,17 +487,20 @@ int sequencer_commit(const char *defmsg, struct 
> > replay_opts *opts,
> >  
> > if (IS_REBASE_I()) {
> > env = read_author_script();
> > -   if (!env)
> > +   if (!env) {
> > +   const char *gpg_opt = gpg_sign_opt_quoted(opts);
> > +
> > return error("You have staged changes in your working "
> > "tree. If these changes are meant to be\n"
> > "squashed into the previous commit, run:\n\n"
> > -   "  git commit --amend $gpg_sign_opt_quoted\n\n"
> 
> How did this get expanded by error(), and why we want to replace
> it if it works?

It did not work. It was a place-holder waiting for this patch ;-)

> 
> > +   "  git commit --amend %s\n\n"
> > "If they are meant to go into a new commit, "
> > "run:\n\n"
> > -   "  git commit $gpg_sign_opt_quoted\n\n"
> > +   "  git commit %s\n\n"
> > "In both case, once you're done, continue "
> > "with:\n\n"
> > -   "  git rebase --continue\n");
> > +   "  git rebase --continue\n", gpg_opt, gpg_opt);
> 
> Instead of passing option twice, why not make use of %1$s (arg reordering),
> that is
> 
>   +   "  git commit --amend %1$s\n\n"
> [...]
>   +   "  git commit %1$s\n\n"

Cute. But would this not drive the l10ners insane?

> So shell quoting is required only for error output.

Indeed.

> > @@ -955,8 +974,27 @@ static int populate_opts_cb(const char *key, const 
> > char *value, void *data)
> >  
> >  static int read_populate_opts(struct replay_opts *opts)
> >  {
> > -   if (IS_REBASE_I())
> > +   if (IS_REBASE_I()) {
> > +   struct strbuf buf = STRBUF_INIT;
> > +
> > +   if (read_oneliner(, rebase_path_gpg_sign_opt(), 1)) {
> > +   if (buf.len && buf.buf[buf.len - 1] == '\n') {
> > +   if (--buf.len &&
> > +   buf.buf[buf.len - 1] == '\r')
> > +   buf.len--;
> > +   buf.buf[buf.len] = '\0';
> > +   }
> 
> Isn't there some strbuf_chomp() / strbuf_strip_eof() function?
> Though as strbuf_getline() uses something similar...

Even worse. read_oneliner() *already* does that. I just forgot to delete
this code when I introduced and used read_oneliner().

Thanks.

> > +   if (!starts_with(buf.buf, "-S"))
> > +   strbuf_reset();
> 
> Should we signal that there was problem with a file contents?

Maybe. But probably not: this file is written by git-rebase itself. I
merely safe-guarded against empty files here.

> > +   else {
> > +   opts->gpg_sign = buf.buf + 2;
> > +   strbuf_detach(, NULL);
> 
> Wouldn't we leak 2 characters that got skipped?  Maybe xstrdup would
> be better (if it is leaked, and not reattached)?

We do not leak anything because I changed the code locally already to use
sequencer_entrust() (I guess in response to an earlier of your comments).

Ciao,
Dscho

Re: [PATCH 16/22] sequencer: prepare for rebase -i's GPG settings

[Johannes Schindelin]

Hi Kuba,

On Wed, 31 Aug 2016, Jakub Narębski wrote:

> Note that if the last line was
> 
> + sequencer_entrust(opts, strbuf_detach(, 
> NULL));
> 
> we can make it not leak.  A bit tricksy, though.

Heh... I made it that tricksy, then.

Ciao,
Dscho

Re: [PATCH 13/22] sequencer: remember the onelines when parsing the todo file

[Johannes Schindelin]

Hi Junio,

On Wed, 31 Aug 2016, Junio C Hamano wrote:

> Jakub Narębski  writes:
> 
>  @@ -709,6 +709,8 @@ static int read_and_refresh_cache(struct replay_opts 
>  *opts)
>   struct todo_item {
>   enum todo_command command;
>   struct commit *commit;
>  +const char *arg;
>  +int arg_len;
> >  
> >> I am not sure what the "commit" field of type "struct commit *" is
> >> for.  It is not needed until it is the commit's turn to be picked or
> >> reverted; if we end up stopping in the middle, parsing the commit
> >> object for later steps will end up being wasted effort.
> >
> > From what I understand this was what sequencer did before this
> > series, so it is not a regression (I think; the commit parsing
> > was in different function, but I think at the same place in
> > the callchain).
> 
> Yes, I agree with you and I didn't mean "this is a new bug" at all.
> It just is an indication that further refactoring after this step is
> needed, and it is likely to involve removal or modification of this
> field.

Not so. After you review the sequencer-i patches, you will see that it
makes tons of sense to have that "commit" field: "pick" is by far the most
common case, and it would not make sense at all to validate the todo
script, only to throw away the information we got, only to re-gain it
later in the sequencer.

Read: I strongly disagree with your cursory verdict that the "commit"
field should go.

Ciao,
Dscho

Re: [PATCH 13/22] sequencer: remember the onelines when parsing the todo file

[Johannes Schindelin]

Hi Kuba & Junio,

On Wed, 31 Aug 2016, Jakub Narębski wrote:

> W dniu 31.08.2016 o 21:10, Junio C Hamano pisze:
> > Jakub Narębski  writes:
> > 
> >>> diff --git a/sequencer.c b/sequencer.c
> >>> index 06759d4..3398774 100644
> >>> --- a/sequencer.c
> >>> +++ b/sequencer.c
> >>> @@ -709,6 +709,8 @@ static int read_and_refresh_cache(struct replay_opts 
> >>> *opts)
> >>>  struct todo_item {
> >>>   enum todo_command command;
> >>>   struct commit *commit;
> >>> + const char *arg;
> >>> + int arg_len;
>  
> > I am not sure what the "commit" field of type "struct commit *" is
> > for.  It is not needed until it is the commit's turn to be picked or
> > reverted; if we end up stopping in the middle, parsing the commit
> > object for later steps will end up being wasted effort.
> 
> From what I understand this was what sequencer did before this
> series, so it is not a regression (I think; the commit parsing
> was in different function, but I think at the same place in
> the callchain).

Exactly.

> > Also, when the sequencer becomes one sequencer to rule them all, the
> > command set may contain something that does not even mention any
> > commit at all (think: exec).
> 
> The "exec" line is a bit of exception, all other rebase -i commands
> take commit as parameter.  It could always use NULL.

There is also "noop".

> > So I am not sure if we want a parsed commit there (I would not
> > object if we kept the texual object name read from the file,
> > though).  The "one sequencer to rule them all" may even have to say
> > "now give name ':1' to the result of the previous operation" in one
> > step and in another later step have an instruction "merge ':1'".
> > When that happens, you cannot even pre-populate the commit object
> > when the sequencer reads the file, as the commit has not yet been
> > created at that point.
> 
> True, --preserve-merges rebase is well, different.

It is mis-designed. And I can be that harsh because it was my design.

In the meantime I came up with a much better design, and implemented it as
a shell script on top of rebase -i. Since shell scripts run like slow
molasses, even more so on Windows, I have a loose plan to implement its
functionality as a new --recreate-merges option, and to deprecate
--preserve-merges when that new option works.

It needs to be a new option (not a --preserve-merges=v2) because it is a
totally different beast. For starters, it does not need its own code path
that overrides pick_one, as --preserve-merges does.

But I get way ahead of myself. First we need to get these last few bits
and pieces in place to accelerate (non --preserve-merges) rebase -i.

Ciao,
Dscho

Re: [PATCH 00/34] Teach the sequencer to act as rebase -i's backend

[Johannes Schindelin]

Hi Dennis,

On Wed, 31 Aug 2016, Dennis Kaarsemaker wrote:

> On wo, 2016-08-31 at 10:53 +0200, Johannes Schindelin wrote:
> > This marks the count down to '3': two more patch series after this
> > (really tiny ones) and we have a faster rebase -i.
> 
> I got to 16/34 (and skipped 07/34), will continue tomorrow. I hope the
> comments are useful.

Much appreciated!
Dscho

Re: [PATCH 15/34] sequencer (rebase -i): leave a patch upon error

[Johannes Schindelin]

Hi Dennis,

On Wed, 31 Aug 2016, Dennis Kaarsemaker wrote:

> On wo, 2016-08-31 at 10:55 +0200, Johannes Schindelin wrote:
> > Just like the interactive rebase, we want to leave a 'patch' file for
> > further inspection by the user (even if we never tried to actually apply
> > that patch, since we're cherry-picking instead).
> > 
> > Signed-off-by: Johannes Schindelin 
> 
> This commit message confuses me. Did you mean s/Just like the/When
> doing an/? 

What I meant is this: when calling `git rebase -i` right now, i.e. before
any of my rebase--helper work, a failing rebase will leave a `patch` file
in the `.git/rebase-merge/` directory. Since the sequencer is in the
process of learning how to do an interactive rebase, it needs to learn the
same trick.

But I guess that your suggested edit makes things much clearer.

Thanks,
Dscho

Re: Feature Request: Branch-Aware Submodules

[Hedges Alexander]

Since I don’t want to change any history in the subproject, to me the most 
expected behavior would be:

git submodule update —-recursive

with submodule.*.update set to the command:

```
#!/bin/bash

branches=`git branch --points-at "$1"`

if [ ! $branches ] ; then
git checkout "$1"
echo "do normal checkout"
else
points_to_master=
other_branch=
for b in $branches ; do
if [ "$b" = "master" ] ; then
points_to_master="true"
else
other_branch="$b"
fi
done
if [ points_to_master ] ; then
git checkout master
else
git checkout "$other_branch"
fi
fi
```

Now, this is not perfect and I’m sure I’ll refine it whenever I find it doesn’t
suit my needs, but I’m sure you can see the intentions here. I’m also not quite
sure whether to prioritize tags over branches or the other way around.

Thanks for the suggestion. I hope this or a similar behavior could sometime
become the default in git. Until the suggested quick fix will do for me.

Best Regards,
Alexander Hedges

> On 29 Aug 2016, at 04:17, Jacob Keller  wrote:
> 
> On Fri, Aug 26, 2016 at 8:12 AM, Hedges  Alexander
>  wrote:
>>> On 25 Aug 2016, at 19:45, Stefan Beller  wrote:
>>> [1] https://github.com/jlehmann/git-submod-enhancements
>>> which has some attempts for checkout including the submodules.
>>> I also tried writing some patches which integrate checking out submodules
>>> via checkout as well. A quicker `solution` would be a config option that
>>> just runs `git submodule update` after each checkout/pull etc.
>>> 
>> 
>> I see. The quick fix is almost what I’m looking for, except that it leaves
>> the repo in a detached head state. Could the submodule update be made
>> automatically and intelligently pick the branch?
>> 
> 
> You probably want "git submodule update --rebase" or "git submodule
> update --merge" See git help submodule under the update section, or
> even a custom command variant where you can write your own bit of
> shell that does what your project expects.
> 
> Thanks,
> Jake

Re: [PATCH 22/22] sequencer: refactor write_message()

[Jakub Narębski]

Hello Johannes,

W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:

> The write_message() function safely writes an strbuf to a file.
> Sometimes this is inconvenient, though: the text to be written may not
> be stored in a strbuf, or the strbuf should not be released after
> writing.

By "this" you mean "using strbuf", isn't it?  It is not very obvious,
and I think it would be better to say it explicitly.

>
> Let's allow for such use cases by refactoring write_message() to allow
> for a convenience function write_file_gently(). As some of the upcoming
> callers of that new function will want to append a newline character,
> let's just add a flag for that, too.

This paragraph feels a bit convoluted.

As I understand it, you refactor "safely writing string to a file"
into write_with_lock_file(), and make write_message() use it.  The
new function makes it easy to create new convenience function 
write_file_gently(); as some of the upcoming callers of this new
function would want to append a newline character, add a flag for
it in write_file_gently(), and thus in write_with_lock_file().

Isn't it better / easier to understand?

> 
> Signed-off-by: Johannes Schindelin 
> ---
>  sequencer.c | 21 ++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 5efed2e..f5b5e5e 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -239,22 +239,37 @@ static void print_advice(int show_hint, struct 
> replay_opts *opts)
>   }
>  }
>  
> -static int write_message(struct strbuf *msgbuf, const char *filename)
> +static int write_with_lock_file(const char *filename,
> + const void *buf, size_t len, int append_eol)
>  {
>   static struct lock_file msg_file;
>  
>   int msg_fd = hold_lock_file_for_update(_file, filename, 0);
>   if (msg_fd < 0)
>   return error_errno(_("Could not lock '%s'"), filename);
> - if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
> + if (write_in_full(msg_fd, buf, len) < 0)
>   return error_errno(_("Could not write to %s"), filename);

You could have, for consistency, add quotes around filename (see previous
error_errno callsite), *while at it*:

return error_errno(_("Could not write to '%s'"), filename);


> - strbuf_release(msgbuf);
> + if (append_eol && write(msg_fd, "\n", 1) < 0)
> + return error_errno(_("Could not write eol to %s"), filename);

Same here, and it wouldn't even be 'while at it'

  + return error_errno(_("Could not write eol to '%s'"), filename);


>   if (commit_lock_file(_file) < 0)
>   return error(_("Error wrapping up %s."), filename);

Another "while at it"... though the one that can be safely postponed
(well, the make message easier to understand part, not the quote
filename part):

return error(_("Error wrapping up writing to '%s'."), filename);


>  
>   return 0;
>  }
>  
> +static int write_message(struct strbuf *msgbuf, const char *filename)
> +{
> + int res = write_with_lock_file(filename, msgbuf->buf, msgbuf->len, 0);
> + strbuf_release(msgbuf);
> + return res;
> +}

Nice.

> +
> +static int write_file_gently(const char *filename,
> +  const char *text, int append_eol)
> +{
> + return write_with_lock_file(filename, text, strlen(text), append_eol);
> +}

Nice.  And it is static function, so we don't need to come up
with a better function name (to describe its function better).

> +
>  /*
>   * Reads a file that was presumably written by a shell script, i.e.
>   * with an end-of-line marker that needs to be stripped.
> 

And thus we got to the last patch in this series.  I have skipped
patches that already got reviewed; are there some that you would
like to have second review of?  Is there patch series that needs
to be applied earlier that needs a review?

P.S. I'll try to respond to your comments later today.

Regards,
-- 
Jakub Narębski

Re: [PATCH 21/22] sequencer: left-trim the lines read from the script

[Jakub Narębski]

Hello Johannes,

W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:

Subject: [PATCH 21/22] sequencer: left-trim the lines read from the script

In the subject, it should probably be without "the", as "lines"
are plural.

s/left-trim the lines/left-trim lines/

> Interactive rebase's scripts may be indented; We need to handle this
> case, too, now that we prepare the sequencer to process interactive
> rebases.

s/; We need/; we need/

Nice little bit of scaffolding for sequencer-izing rebase -i.

> 
> Signed-off-by: Johannes Schindelin 
> ---
>  sequencer.c | 3 +++
>  1 file changed, 3 insertions(+)

Small change, easy to review.

> 
> diff --git a/sequencer.c b/sequencer.c
> index 0614b90..5efed2e 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -864,6 +864,9 @@ static int parse_insn_line(struct todo_item *item, const 
> char *bol, char *eol)
>   char *end_of_object_name;
>   int i, saved, status, padding;
>  
> + /* left-trim */
> + bol += strspn(bol, " \t");
> +

Nice.  Thanks for the comment.  "left-trim" is better than "de-indent".

'bol' is beginning-of-line, isn't it (a complement to eol)?

>   for (i = 0; i < ARRAY_SIZE(todo_command_strings); i++)
>   if (skip_prefix(bol, todo_command_strings[i], )) {
>   item->command = i;
> 

-- 
Jakub Narębski

Re: [PATCH 19/22] sequencer: support cleaning up commit messages

[Jakub Narębski]

Hello Johannes,

W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:

> The sequencer_commit() function already knows how to amend commits, and
> with this new option, it can also clean up commit messages (i.e. strip
> out commented lines). This is needed to implement rebase -i's 'fixup'
> and 'squash' commands as sequencer commands.
> 
> Signed-off-by: Johannes Schindelin 
> ---
>  sequencer.c | 10 +++---
>  sequencer.h |  3 ++-
>  2 files changed, 9 insertions(+), 4 deletions(-)

This looks like nice little piece of enhancement, building scaffolding
for sequencer-izing interactive rebase bit by bit.

> 
> diff --git a/sequencer.c b/sequencer.c
> index 20f7590..5ec956f 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -478,7 +478,8 @@ static char **read_author_script(void)
>   * (except, of course, while running an interactive rebase).
>   */
>  int sequencer_commit(const char *defmsg, struct replay_opts *opts,
> -   int allow_empty, int edit, int amend)
> +   int allow_empty, int edit, int amend,
> +   int cleanup_commit_message)

All right, though it slowly begins coming close to the threshold
where using bitfield flags would be sensible.

>  {
>   char **env = NULL;
>   struct argv_array array;
> @@ -515,9 +516,12 @@ int sequencer_commit(const char *defmsg, struct 
> replay_opts *opts,
>   argv_array_push(, "-s");
>   if (defmsg)
>   argv_array_pushl(, "-F", defmsg, NULL);
> + if (cleanup_commit_message)
> + argv_array_push(, "--cleanup=strip");

Good.

>   if (edit)
>   argv_array_push(, "-e");
> - else if (!opts->signoff && !opts->record_origin &&
> + else if (!cleanup_commit_message &&

All right, explicit cleanup=strip overrides "commit.cleanup" config,
and turns off passing commit verbatim (incompatible with stripping)...

> +  !opts->signoff && !opts->record_origin &&

...adding signoff and recording origin requires not passing commit
verbatim,...

>git_config_get_value("commit.cleanup", ))

..., and in other cases are check the "commit.cleanup"...

>   argv_array_push(, "--cleanup=verbatim");

... and pass commit verbatim if it is not set.


Ah, well, the change you made looks good.

>  
> @@ -781,7 +785,7 @@ static int do_pick_commit(enum todo_command command, 
> struct commit *commit,
>   }
>   if (!opts->no_commit)
>   res = sequencer_commit(opts->edit ? NULL : git_path_merge_msg(),
> - opts, allow, opts->edit, 0);
> + opts, allow, opts->edit, 0, 0);

The calling convention begins to look unwieldy, but we have only
a single such callsite, and there are quite a bit callsites in
Git code that have similar API ("git grep ', 0, 0' -- '*.c'").
So we don't need to think about alternatives.  Yet.

It's a pity that emulation of named parameters in C requires
relying on designated inits from C99

  typedef struct {
double pressure, moles, temp;
  } ideal_struct;

  #define ideal_pressure(...) ideal_pressure_base((ideal_struct){.pressure=1,   
\
.moles=1, .temp=273.15, __VA_ARGS__})

  double ideal_pressure_base(ideal_struct in)
  {
return 8.314 * in.moles*in.temp/in.pressure;
  }

  ... ideal_pressure(.moles=2, .temp=373.15) ...

>  
>  leave:
>   free_message(commit, );
> diff --git a/sequencer.h b/sequencer.h
> index 2106c0d..e272549 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -50,7 +50,8 @@ int sequencer_rollback(struct replay_opts *opts);
>  int sequencer_remove_state(struct replay_opts *opts);
>  
>  int sequencer_commit(const char *defmsg, struct replay_opts *opts,
> -   int allow_empty, int edit, int amend);
> +   int allow_empty, int edit, int amend,
> +   int cleanup_commit_message);
>  
>  extern const char sign_off_header[];
>  
> 

Re: [PATCH 13/22] sequencer: remember the onelines when parsing the todo file

[Johannes Schindelin]

Hi Junio,

On Wed, 31 Aug 2016, Junio C Hamano wrote:

> Jakub Narębski  writes:
> 
> >> diff --git a/sequencer.c b/sequencer.c
> >> index 06759d4..3398774 100644
> >> --- a/sequencer.c
> >> +++ b/sequencer.c
> >> @@ -709,6 +709,8 @@ static int read_and_refresh_cache(struct replay_opts 
> >> *opts)
> >>  struct todo_item {
> >>enum todo_command command;
> >>struct commit *commit;
> >> +  const char *arg;
> >> +  int arg_len;
> >
> > Why 'arg', and not 'oneline', or 'subject'?
> > I'm not saying it is bad name.
> 
> I am not sure what the "commit" field of type "struct commit *" is
> for.  It is not needed until it is the commit's turn to be picked or
> reverted; if we end up stopping in the middle, parsing the commit
> object for later steps will end up being wasted effort.

No, it won't be wasted effort, as we *validate* the todo script this way.
And since we may very well need the info later (most rebases do not fail
in the middle), we store it, too.

> Also, when the sequencer becomes one sequencer to rule them all, the
> command set may contain something that does not even mention any
> commit at all (think: exec).

Right, in which case the "commit" field will have the value... *drum
roll*... NULL!

> So I am not sure if we want a parsed commit there (I would not
> object if we kept the texual object name read from the file,
> though).  The "one sequencer to rule them all" may even have to say
> "now give name ':1' to the result of the previous operation" in one
> step and in another later step have an instruction "merge ':1'".
> When that happens, you cannot even pre-populate the commit object
> when the sequencer reads the file, as the commit has not yet been
> created at that point.

These considerations are pretty hypothetical. I would even place a bet
that we will *never* have ":1" as names, not if I have anything to say...
;-)

What is not so hypothetical is that after parsing the todo_list, we will
have to act on the information contained therein. For example we will have
to cherry-pick some of the indicated commits (requiring a struct commit *
for use in do_pick_commit()). Another example: we may need to determine
the oneline for use in fixup!/squash! reordering.

So: keeping *that* aspect of the previous todo_list parsing, i.e. store a
pointer to the already-parsed commit, is the right thing to do.

Ciao,
Dscho

Re: [PATCH 06/34] sequencer (rebase -i): write the 'done' file

[Johannes Schindelin]

Hi Dennis,

On Wed, 31 Aug 2016, Dennis Kaarsemaker wrote:

> On wo, 2016-08-31 at 10:54 +0200, Johannes Schindelin wrote:
> > In the interactive rebase, commands that were successfully processed are
> > not simply discarded, but appended to the 'done' file instead. This is
> > used e.g. to display the current state to the user in the output of
> > `git status` or the progress.
> 
> Wouldn't it make more sense to have this patch before the ones that
> implement the actual rebase commands?

I waffled about the order so many times that I don't know anymore. The
thing is, while the sequencer is taught incrementally to understand all of
the rebase -i functionality, rebase -i itself is not touched, on purpose.

In the case of the "done" file, my thoughts were: the commands do not need
this file *at all*. In fact, if we did not write the "done" file at all,
the only two types of test failures in the test suite would be 1) git
status' output and 2) the prompt testing for the progress.

So you see, functionally, the "done" file is only relevant to the progress
part of the patch series.

As such, I'd rather keep this patch in the current place, just before
introducing the progress.

> Hmm, and after reading more of this series, I think the same applies to
> some other patches too, e.g. 08/34 and 14/34, so I'm probably missing
> something. So before I make a fool of myself and suggest that the
> implementation of the actual commands should come at the end, maybe you
> could tell me what I'm missing :) 

No, no, don't hesitate to suggest reorderings. I am really thankful for
the discussion we are having, so that the outcome is better than what I
have right now. If the outcome would be the very same patches, but with
more confidence, it would still be better than what I have right now ;-)

Ciao,
Dscho

Re: [PATCH 13/22] sequencer: remember the onelines when parsing the todo file

[Johannes Schindelin]

Hi Kuba,

On Wed, 31 Aug 2016, Jakub Narębski wrote:

> W dniu 29.08.2016 o 10:05, Johannes Schindelin pisze:
> 
> > The `git-rebase-todo` file contains a list of commands. Most of those
> > commands have the form
> > 
> >   
> > 
> > The  is displayed primarily for the user's convenience, as
> > rebase -i really interprets only the   part. However, there
> > are *some* places in interactive rebase where the  is used to
> > display messages, e.g. for reporting at which commit we stopped.
> > 
> > So let's just remember it when parsing the todo file; we keep a copy of
> > the entire todo file anyway (to write out the new `done` and
> > `git-rebase-todo` file just before processing each command), so all we
> > need to do is remember the begin and end offsets.
> 
> Actually what we remember is pointer and length, or begin offset and length,
> not offset and offset.

Right. Fixed.

> > diff --git a/sequencer.c b/sequencer.c
> > index 06759d4..3398774 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -709,6 +709,8 @@ static int read_and_refresh_cache(struct replay_opts 
> > *opts)
> >  struct todo_item {
> > enum todo_command command;
> > struct commit *commit;
> > +   const char *arg;
> > +   int arg_len;
> 
> Why 'arg', and not 'oneline', or 'subject'?
> I'm not saying it is bad name.

Because we will use it for `exec` commands' args, too. Clarified in the
commit message.

> > @@ -760,6 +762,9 @@ static int parse_insn_line(struct todo_item *item, 
> > const char *bol, char *eol)
> > status = get_sha1(bol, commit_sha1);
> > *end_of_object_name = saved;
> >  
> > +   item->arg = end_of_object_name + strspn(end_of_object_name, " \t");
> > +   item->arg_len = (int)(eol - item->arg);
> > +
> 
> Does it work correctly for line without , that is
> 
>
> 
> I think it does, but I not entirely sure.

It does work correctly: in the example, *end_of_object_name would be '\n',
and strspn(end_of_object_name, " \t") would return 0.

Thanks for the review!
Dscho

Re: [PATCH 27/34] sequencer (rebase -i): differentiate between comments and 'noop'

[Dennis Kaarsemaker]

On wo, 2016-08-31 at 10:56 +0200, Johannes Schindelin wrote:
> diff --git a/sequencer.c b/sequencer.c
> index 51c2f76..4c902e5 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -763,7 +763,8 @@ enum todo_command {
> TODO_SQUASH,
> TODO_EXEC,
> TODO_NOOP,
> -   TODO_DROP
> +   TODO_DROP,
> +   TODO_COMMENT
>  };

(picking a random commit that touches this enum)

In a few places you now make comparisons like "< TODO_NOOP", so I think
it would be good to have a comment near the definition of this enum
that says that ordering matters and why, so people don't attempt to add
a new TODO_FOOBAR at the end.

D. 

Re: [PATCH 32/34] sequencer (rebase -i): show the progress

[Dennis Kaarsemaker]

On wo, 2016-08-31 at 10:56 +0200, Johannes Schindelin wrote:
> The interactive rebase keeps the user informed about its progress.
> If the sequencer wants to do the grunt work of the interactive
> rebase, it also needs to show that progress.
> 
> Signed-off-by: Johannes Schindelin 
> ---
>  sequencer.c | 33 +
>  1 file changed, 33 insertions(+)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 89fd625..e8c6daf 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1218,6 +1218,7 @@ struct todo_list {
>   struct strbuf buf;
>   struct todo_item *items;
>   int nr, alloc, current;
> + int done_nr, total_nr;
>  };
>  
>  #define TODO_LIST_INIT { STRBUF_INIT }
> @@ -1329,6 +1330,17 @@ static int parse_insn_buffer(char *buf, struct 
> todo_list *todo_list)
>   return res;
>  }
>  
> +static int count_commands(struct todo_list *todo_list)
> +{
> + int count = 0, i;
> +
> + for (i = 0; i < todo_list->nr; i++)
> + if (todo_list->items[i].command != TODO_COMMENT)
> + count++;
> +
> + return count;
> +}
> +
>  static int read_populate_todo(struct todo_list *todo_list,
>   struct replay_opts *opts)
>  {
> @@ -1355,6 +1367,22 @@ static int read_populate_todo(struct todo_list 
> *todo_list,
>   if (!todo_list->nr &&
>   (!is_rebase_i(opts) || !file_exists(rebase_path_done(
>   return error(_("No commits parsed."));
> +
> + if (is_rebase_i(opts)) {
> + struct todo_list done = TODO_LIST_INIT;
> +
> + if (strbuf_read_file(, rebase_path_done(), 0) > 0 &&
> + !parse_insn_buffer(done.buf.buf, ))
> + todo_list->done_nr = count_commands();
> + else
> + todo_list->done_nr = 0;
> +
> + todo_list->total_nr = todo_list->done_nr
> + + count_commands(todo_list);
> +
> + todo_list_release();
> + }
> +
>   return 0;
>  }
>  
> @@ -1900,6 +1928,11 @@ static int pick_commits(struct todo_list *todo_list, 
> struct replay_opts *opts)
>   if (save_todo(todo_list, opts))
>   return -1;
>   if (is_rebase_i(opts)) {
> + if (item->command != TODO_COMMENT)
> + fprintf(stderr, "Rebasing (%d/%d)%s",
> + ++(todo_list->done_nr),
> + todo_list->total_nr,
> + opts->verbose ? "\n" : "\r");
>   unlink(rebase_path_message());
>   unlink(rebase_path_author_script());
>   unlink(rebase_path_stopped_sha());

(picking a random commit that shows this 'symptom')

You're sprinking a lot of is_rebase_i's around sequencer.c to make sure
there are no changes in behaviour. I wonder if the right balance has
been struck between 'no changes in behaviour' and 'common behaviour'.
For instance, in this case, maybe it would be a better idea for non-
rebase uses of the sequencer to also show progress.

D.

Re: [PATCH 07/34] sequencer (rebase -i): add support for the 'fixup' and 'squash' commands

[Dennis Kaarsemaker]

On wo, 2016-08-31 at 10:54 +0200, Johannes Schindelin wrote:

> +static int is_fixup(enum todo_command command)
> +{
> + return command == TODO_FIXUP || command == TODO_SQUASH;
> +}

It sounds wrong to have a function named is_fixup return true when the
command isn't a fixup but a squash. Maybe name it
changes_previous_commit or something?

> +static const char *nth_for_number(int n)
> +{
> + int n1 = n % 10, n10 = n % 100;
> +
> + if (n1 == 1 && n10 != 11)
> + return "st";
> + if (n1 == 2 && n10 != 12)
> + return "nd";
> + if (n1 == 3 && n10 != 13)
> + return "rd";
> + return "th";
> +}

>8---

> + if (command == TODO_SQUASH) {
> + unlink(rebase_path_fixup_msg());
> + strbuf_addf(, "\n%c This is the %d%s commit message:\n\n%s",
> + comment_line_char,
> + count, nth_for_number(count), body);
> + }
> + else if (command == TODO_FIXUP) {
> + strbuf_addf(,
> + "\n%c The %d%s commit message will be skipped:\n\n",
> + comment_line_char, count, nth_for_number(count));
> + strbuf_add_commented_lines(, body, strlen(body));
> + }

This way of handling numbers is not translatable, and I really think we
should mark these strings for translation, like they are in the .sh
version.

D.

Re: [PATCH v13 00/14] libify apply and use lib in am, part 3

[Christian Couder]

On Thu, Sep 1, 2016 at 12:15 AM, Junio C Hamano  wrote:
> Christian Couder  writes:
>
>> On Mon, Aug 29, 2016 at 9:04 PM, Junio C Hamano  wrote:
>>> Christian Couder  writes:
>>>
 Highlevel view of the patches in the series
 ~~~

 This is "part 3" of the full patch series. I am resending only the
 last 14 patches of the full series as "part 3", because I don't want
 to resend the first 27 patches of v10 nearly as is.
>>>
>>> Just to make sure, you are sending the first 11 of these 14 exactly
>>> as-is, right?  I didn't spot anything different other than 12 and 13
>>> that replaced the "alternate index" step from the previous round.
>>
>> Yeah, the first 11 of the 14 patches have no change compared to v12.
>> I didn't want to create a "part 4" as that could be confusing, and
>> sending the first 11 patches gives them another chance to be reviewed
>> again.
>
> Hmph.
>
> But most likely, you made sure that those who _could_ review the
> first 11 are miniscule minority by omitting the earlier steps before
> these 14 patches -- unless they are familiar with them, the first 11
> patches are not much use to them.  And those who are familiar have
> already seen the first 11, too.  That was why I wondered who the
> target audience was when seeing only the last 14, among which 11 of
> them were identical to the previous.

Following Stefan's review, it looks like I will need to resend at
least 02/14, 10/14 and 14/14.
What do you prefer me to resend:
1) all the last 40 or so patches
2) the last 14 patches
3) only the few patches that changed
?

Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests

[Johannes Schindelin]

Hi Sverre,

On Wed, 31 Aug 2016, Sverre Rabbelier wrote:

> On Wed, Aug 31, 2016 at 3:36 AM Johannes Schindelin
>  wrote:
> > On Tue, 30 Aug 2016, Junio C Hamano wrote:
> > > Jeff King  writes:
> > > > Hmm, interesting. Your approach seems reasonable, but I have to wonder
> > > > if writing the pid in the first place is sane.
> > > >
> > > > I started to write up my reasoning in this email, but realized it was
> > > > rapidly becoming the content of a commit message. So here is that
> > > > commit.
> > >
> > > Sounds sensible; if this makes Dscho's "which ones failed in the
> > > previous run" simpler, that is even better ;-)
> >
> > I did not have the time to dig further before now. There must have been a
> > good reason why we append the PID.
> >
> > Sverre, you added that code in 2d84e9f (Modify test-lib.sh to output stats
> > to t/test-results/*, 2008-06-08): any idea why the - suffix was
> > needed?
> 
> I can't really recall, but I think it may have been related to me
> doing something like this:
> 1. Make a change, and start running tests (this takes a long time)
> 2. Notice a failure, start fixing it, leave tests running to find
> further failures
> 3. Finish fix, first tests are still running, start another run in a
> new terminal (possibly of just the one failed test I was fixing) to
> see if the fix worked.
> 
> Without the pid, the second run would clobber the results from the first run.
> 
> 
> If only past-me was more rigorous about writing good commit messages :P.

:-)

Would present-you disagree with stripping off the - suffix, based on
your recollections?

Ciao,
Dscho

Re: [PATCH v13 10/14] apply: change error_routine when silent

[Christian Couder]

On Thu, Sep 1, 2016 at 12:20 AM, Stefan Beller  wrote:
> On Sat, Aug 27, 2016 at 11:45 AM, Christian Couder
>  wrote:
>> To avoid printing anything when applying with
>> `state->apply_verbosity == verbosity_silent`, let's save the
>> existing warn and error routines before applying, and let's
>> replace them with a routine that does nothing.
>>
>> Then after applying, let's restore the saved routines.
>>
>> Helped-by: Stefan Beller 
>> Signed-off-by: Christian Couder 
>> ---
>>  apply.c | 21 -
>>  apply.h |  8 
>>  2 files changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/apply.c b/apply.c
>> index ddbb0a2..bf81b70 100644
>> --- a/apply.c
>> +++ b/apply.c
>> @@ -112,6 +112,11 @@ void clear_apply_state(struct apply_state *state)
>> /* >fn_table is cleared at the end of apply_patch() */
>>  }
>>
>> +static void mute_routine(const char *bla, va_list params)
>
> Instead of 'bla' you could go with 'format' as the man page for
> [f]printf puts it.
> Or you could leave it empty, i.e.
>
> static void mute_routine(const char *, va_list)
> ...

Ok to do that.

> I personally do not mind bla, as I know that the first parameter is
> supposed to be the format, but let's not add unneeded information.
> (Side question: Is there a culture that doesn't recognize 'bla' as a
> fill word?)
>
>> @@ -4864,7 +4876,7 @@ int apply_all_patches(struct apply_state *state,
>> state->newfd = -1;
>> }
>>
>> -   return !!errs;
>> +   res = !!errs;
>
> I am trying to understand this and the next chunk (they work together?)

Yes, they work together.

>>  end:
>> if (state->newfd >= 0) {
>> @@ -4872,5 +4884,12 @@ int apply_all_patches(struct apply_state *state,
>> state->newfd = -1;
>> }
>>
>> +   if (state->apply_verbosity <= verbosity_silent) {
>> +   set_error_routine(state->saved_error_routine);
>> +   set_warn_routine(state->saved_warn_routine);
>> +   }
>> +
>> +   if (res > -1)
>> +   return res;
>> return (res == -1 ? 1 : 128);
>
> So anything > -1 is returned as is, and == -1 returns 1 and <-1  returns 128 ?
>
> So I guess a reminder/explanation on why we need to fiddle with return codes
> in the commit message would help. (I do not understand how the
> verbosity influences return codes.)

It doesn't influence return codes, but we need to restore error
routines just before returning, so we cannot return early anymore.
I will add something to the commit message.

Re: [PATCH 08/22] sequencer: remove overzealous assumption

[Johannes Schindelin]

Hi Junio,

On Wed, 31 Aug 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > So if you must have a patch that disagrees with this overzealous
> > check, the "revamp todo parsing" one is probably the first. But it is
> > better to think of this at a higher level than just patches: it is
> > wrong to limit the todo script to contain only identical commands.
> 
> So if you think of this at even higher level, the check done in
> parse_insn_line() that _assumes_ that opts->action must match the
> actions on each line is _WRONG_, but what this test expects to see is
> perfectly reasonable, I would think.
> 
> It is a different matter if it makes sense to _verify_ that the user
> didn't make nonsensical change to the generated insn and error out when
> s/he did.  I tend to think it is pointless, and I wouldn't object if
> this test is removed due to that reason.

Fine. I will reintroduce that check. I guess I have the time ;-)

Ciao,
Dscho

Re: [PATCH v13 02/14] apply: rename and move opt constants to apply.h

[Christian Couder]

On Wed, Aug 31, 2016 at 11:46 PM, Stefan Beller  wrote:
> On Sat, Aug 27, 2016 at 11:45 AM, Christian Couder
>  wrote:
>>  extern int check_apply_state(struct apply_state *state, int force_apply);
>>
>
> With greater scope comes greater responsibility. Nit of the day:
> In case a reroll is necessary, consider putting a comment here.
> (What are these constants? what do they control? How/where do I use them?)
>
>> +#define APPLY_OPT_INACCURATE_EOF   (1<<0)
>> +#define APPLY_OPT_RECOUNT  (1<<1)

Ok I will reroll with some added comments.

Re: [PATCH 08/22] sequencer: remove overzealous assumption

[Johannes Schindelin]

Hi Kuba,

On Wed, 31 Aug 2016, Jakub Narębski wrote:

> W dniu 31.08.2016 o 20:36, Johannes Schindelin pisze:
> 
> I wonder: would 'git cherry-pick --continue' be able to finish
> 'git revert', and vice versa, then?  Or 'git sequencer --continue'?

I just tested this, via

diff --git a/t/t3510-cherry-pick-sequence.sh
b/t/t3510-cherry-pick-sequence.sh
index 96c7640..085d8bc 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -55,7 +55,7 @@ test_expect_success 'cherry-pick
mid-cherry-pick-sequence' '
git checkout HEAD foo &&
git cherry-pick base &&
git cherry-pick picked &&
-   git cherry-pick --continue &&
+   git revert --continue &&
git diff --exit-code anotherpick

(Danger! Whitespace corrupted!!!)

It appears that this passes now.

Probably `git sequencer --continue` would work, too, if there was a `git
sequencer`. :0)

> > On Wed, 31 Aug 2016, Jakub Narębski wrote: 
> >> W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
>  
> >>> diff --git a/t/t3510-cherry-pick-sequence.sh 
> >>> b/t/t3510-cherry-pick-sequence.sh
> >>> index 7b7a89d..6465edf 100755
> >>> --- a/t/t3510-cherry-pick-sequence.sh
> >>> +++ b/t/t3510-cherry-pick-sequence.sh
> >>> @@ -459,17 +459,6 @@ test_expect_success 'malformed instruction sheet 1' '
> >>>   test_expect_code 128 git cherry-pick --continue
> >>>  '
> >>>  
> >>> -test_expect_success 'malformed instruction sheet 2' '
> >>
> >> Hmmm... the description is somewhat lacking (especially compared to
> >> the rest of test), anyway.
> >>
> >> BTW. we should probably rename 'malformed instruction sheet 2'
> >> to 'malformed instruction sheet' if there are no further such
> >> tests after this removal, isn't it?
> > 
> > No, we cannot rename it after this patch because the patch removes it ;-)
> > (It is not a file name but really a label for a test case.)
> 
> Ooops.  What I wanted to say that after removing the test case named
> 'malformed instruction sheet 2' we should also rename *earlier* test
> case from 'malformed instruction sheet 1' to 'malformed instruction sheet',
> as it is now the only 'malformed instruction sheet *' test case.

Actually, you know, I completely missed the fact that there was a
"malformed instruction sheet 3". I renumbered it.

Thanks,
Dscho

Re: [PATCH v13 06/14] apply: make it possible to silently apply

[Christian Couder]

On Thu, Sep 1, 2016 at 12:07 AM, Stefan Beller  wrote:
>> Printing on stdout, and calls to warning() or error() are not
>> taken care of in this patch, as that will be done in following
>> patches.
>
>> -   if (state->apply_verbosely)
>> +   if (state->apply_verbosity > verbosity_normal)
>> error(_("while searching for:\n%.*s"),
>>   (int)(old - oldlines), oldlines);
>
> But this is an error(..) ?

Do you mean that it was a bug in the original code to print this error
only in verbose mode?

> Have you considered to replace all these print functions (error, warning,
> fprintf, printf, fprintf_ln) with another custom
>
> int say_when_at_least(verbosity level, const char *fmt,...)
>
> ? (I guess that would be more invasive, but the result would be more
> consistent.)

My opinion is that there is a reason (or there should have been a
reason) why people decided to use error() instead of warning() for
example.
If I use say_when_at_least(verbosity level, const char *fmt,...) like
you suggest, how do I decide if error() or warning() is used to
actually print the error message?
Another parameter to this function (severity level?) is needed.

Anyway I don't think such a refactoring is needed.

Re: [PATCH 11/22] sequencer: get rid of the subcommand field

[Johannes Schindelin]

Hi Kuba,

On Wed, 31 Aug 2016, Jakub Narębski wrote:

> W dniu 29.08.2016 o 10:05, Johannes Schindelin pisze:
> 
> > While at it, ensure that the subcommands return an error code so that
> > they do not have to die() all over the place (bad practice for library
> > functions...).
> 
> This perhaps should be moved to a separate patch, but I guess
> there is a reason behind "while at it".

Yes. It seemed like the logical thing to do: I already introduce a new
function, why should I shlep over a paradigm I do not want in the end?

> Also subcommand functions no longer are local to sequencer.c

They never were. All you had to do was to set a field and run the global
function.

The real problem there was that the different local functions needed
different parameters, and the round-about way to set those parameters as
fields in a struct and then call a global function with that struct just
makes it impossible to have compile-time safety.

Ciao,
Dscho

Re: [PATCH 10/22] sequencer: avoid completely different messages for different actions

[Johannes Schindelin]

Hi Kuba,

On Wed, 31 Aug 2016, Jakub Narębski wrote:

> CC-ed to Jiang Xin, L10N coordinator.
> 
> W dniu 29.08.2016 o 10:05, Johannes Schindelin pisze:
> 
> > Signed-off-by: Johannes Schindelin 
> > ---
> >  sequencer.c | 7 ++-
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> > 
> > diff --git a/sequencer.c b/sequencer.c
> > index cbdce6d..1b65202 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -232,11 +232,8 @@ static int error_dirty_index(struct replay_opts *opts)
> > if (read_cache_unmerged())
> > return error_resolve_conflict(action_name(opts));
> >  
> > -   /* Different translation strings for cherry-pick and revert */
> > -   if (opts->action == REPLAY_PICK)
> > -   error(_("Your local changes would be overwritten by 
> > cherry-pick."));
> > -   else
> > -   error(_("Your local changes would be overwritten by revert."));
> > +   error(_("Your local changes would be overwritten by %s."),
> > +   action_name(opts));
> 
> If I understand it correctly, it would make "revert" or "cherry-pick"
> untranslated part of error message.  You would need to use translation
> on the result with "_(action_name(opts))", you would have to mark
> todo_command_strings elements for gettext lexicon with N_(...).
> 
> I am rather against this change (see also below).

Okay.

Unfortunately, I have to focus on the correctness of the code at the
moment (and Git for Windows does ship *without* translations for the time
being anyway, mostly to save on space, but also because users complained).

So I will take care of this after v2.10.0.

For the record, how is this supposed to be handled, in particular when I
introduce a new action whose action_name(opts) will be "rebase -i"? Do I
really need to repeat myself three times?

Ciao,
Dscho

Re: [PATCH 09/22] sequencer: completely revamp the "todo" script parsing

[Johannes Schindelin]

Hi Kuba,

On Wed, 31 Aug 2016, Jakub Narębski wrote:

> W dniu 29.08.2016 o 10:05, Johannes Schindelin pisze:
> 
> > When we came up with the "sequencer" idea, we really wanted to have
> > kind of a plumbing equivalent of the interactive rebase. Hence the
> > choice of words: the "todo" script, a "pick", etc.
> > 
> > However, when it came time to implement the entire shebang, somehow this
> > idea got lost and the sequencer was used as working horse for
> > cherry-pick and revert instead. So as not to interfere with the
> > interactive rebase, it even uses a separate directory to store its
> > state.
> > 
> > Furthermore, it also is stupidly strict about the "todo" script it
> > accepts: while it parses commands in a way that was *designed* to be
> > similar to the interactive rebase, it then goes on to *error out* if the
> > commands disagree with the overall action (cherry-pick or revert).
> 
> Does this mean that after the change you would be able to continue
> "git revert" with "git cherry-pick --continue", and vice versa?  Or that
> it would be possible for git-cherry-pick to do reverts (e.g. with ^)?

I guess that I allow that now. Is it harmful? I dunno.

> > Let's just bite the bullet and rewrite the entire parser; the code now
> > becomes not only more elegant: it allows us to go on and teach the
> > sequencer how to parse *true* "todo" scripts as used by the interactive
> > rebase itself. In a way, the sequencer is about to grow up to do its
> > older brother's job. Better.
> 
> Sidenote: this is not your fault, but Git doesn't do a good job on
> changes which are mostly rewrites, trying to match stray '}' and the
> like in generated diff.  I wonder if existing diff heuristic options
> could help here.

I guess --patience would have helped. Or Michael's upcoming
diff-heuristics.

> > While at it, do not stop at the first problem, but list *all* of the
> > problems. This helps the user by allowing to address all issues in
> > one go rather than going back and forth until the todo list is valid.
> 
> That is also a good change, though I wonder how often users need
> to worry about this outside interactive rebase case.  If it is
> preparation for rebase -i, where instruction list is written by
> prone to errors human, it would be nice to have this information
> in the commit message.

Okay.

> > diff --git a/sequencer.c b/sequencer.c
> > index 982b6e9..cbdce6d 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -473,7 +473,26 @@ static int allow_empty(struct replay_opts *opts, 
> > struct commit *commit)
> > return 1;
> >  }
> >  
> > -static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
> > +enum todo_command {
> > +   TODO_PICK,
> > +   TODO_REVERT
> > +};
> 
> Do we have a naming convention for enums elements?  Or are we explicitly
> making enums and #defines interchangeable?  I wonder...
> 
> ...uh, I see we don't have naming convention, but all caps snake-case
> names dominate:
> 
>   $ git grep -A2 'enum .* {'
>   [...]
>   diff.h:enum color_diff {
>   diff.h- DIFF_RESET = 0,
>   diff.h- DIFF_CONTEXT = 1,
>   --
>   dir.c:enum path_treatment {
>   dir.c-  path_none = 0,
>   dir.c-  path_recurse,
>   --
> 
> Shouldn't we say 'TODO_PICK = 0' explicitly, though?

Sure.

> > +static const char *todo_command_strings[] = {
> > +   "pick",
> > +   "revert"
> > +};
> 
> It's a bit pity that we cannot use designated inits, and hanging comma,
> (from ISO C99 standard).  That is:
> 
>   +static const char *todo_command_strings[] = {
>   +   [TODO_PICK]   = "pick",
>   +   [TODO_REVERT] = "revert",
>   +};

I agree, it is a pity. I could do something like I did in fsck.c:

#define FOREACH_TODO_COMMAND(FUNC) \
FUNC(PICK, "pick") \
FUNC(REVERT, "revert")

#define COMMAND_ID(id, string) TODO_##id,
enum todo_command {
FOREACH_TODO_COMMAND(COMMAND_ID)
TODO_END
};
#undef COMMAND_ID

#define COMMAND_ID(id, string) string,
static const char *todo_command_string[] = {
FOREACH_TODO_COMMAND(COMMAND_ID)
NULL
};
#undef COMMAND_ID

However, this is not even readable, let alone any other type of an
improvement. So I won't.

> > @@ -548,7 +568,7 @@ static int do_pick_commit(struct commit *commit, struct 
> > replay_opts *opts)
> 
> From here on changes are about
> 
>   s/opts->action == REPLAY_\(PICK\|REVERT\)/command == TODO_\1/
> 
> Do we still need opts->action, or it is just needed less,
> and it is 'todo' instruction that decides about command
> (as it should)?

We need opts->action. For example, the state directory changes depending
on it: REPLAY_INTERACTIVE_REBASE stores its stuff in
git_path("rebase-merge").

There is lots more behavior that also changes depending on opts->action.

> > [...]
> > if (res) {
> > -   error(opts->action == REPLAY_REVERT
> > +   error(command == TODO_REVERT
> 

Re: [PATCH v13 03/14] Move libified code from builtin/apply.c to apply.{c,h}

[Christian Couder]

On Wed, Aug 31, 2016 at 11:57 PM, Stefan Beller  wrote:
> On Sat, Aug 27, 2016 at 11:45 AM, Christian Couder
>  wrote:
>> As most of the apply code in builtin/apply.c has been libified by a number of
>> previous commits, it can now be moved to apply.{c,h}, so that more code can
>> use it.
>>
>> Helped-by: Nguyễn Thái Ngọc Duy 
>> Helped-by: Ramsay Jones 
>> Signed-off-by: Christian Couder 
>> ---
>>  apply.c | 4731 
>> ++
>>  apply.h |   19 +
>>  builtin/apply.c | 4733 
>> +--
>
> I deduce by roughly the same line count in the .c files, it is just
> moving files over.
> (On my todo list I have an idea how to make reviewing patches like this 
> easier.)

Yeah, at one point I used some special options to try to get a smaller
diff, but it got lost along the way.

>>  3 files changed, 4751 insertions(+), 4732 deletions(-)
>>
>> diff --git a/apply.c b/apply.c
>> index 2eac3e3..7b96130 100644
>> --- a/apply.c
>> +++ b/apply.c
>> @@ -1,5 +1,23 @@
>> +/*
>> + * apply.c
>> + *
>> + * Copyright (C) Linus Torvalds, 2005
>
> We're very inconsistent with the intellectual property log.
> Sometimes we have that at the top of a file, sometimes we don't
> and rather point at the git history to find out who touched the code.
> I'd rather use the history instead of having a bunch of copyright lines.
> So maybe consider to drop this introductory comment as a
> {preparatory, follow up} cleanup?
>
> (This is also a nit that doesn't require a reroll on its own)

Yeah, if people are ok with it I may od it in a follow up patch.

Re: [PATCH v13 14/14] builtin/am: use apply API in run_apply()

[Christian Couder]

On Thu, Sep 1, 2016 at 12:33 AM, Stefan Beller  wrote:
> On Sat, Aug 27, 2016 at 11:45 AM, Christian Couder
>  wrote:
>> +
>> +   if (opts_left != 0)
>> +   die("unknown option passed thru to git apply");
>
> Through and thru are different spellings of the same word.
> Thru is the less preferred form, however, and it might be
> considered out of place outside the most informal contexts.

Ok I will change to "through"...

> [Source: The Internet]
>
> "git grep thru" confirms we only use it in comments or function
> names, both are not exposed to our dear users.

...but I find it strange to use different kind of language for our
users, who by the way are likely to be developers, than for our own
developers.

> The rest looks good.

Thanks for your review,
Christian.

Re: [PATCH 20/34] sequencer (rebase -i): copy commit notes at end

[Dennis Kaarsemaker]

On wo, 2016-08-31 at 10:55 +0200, Johannes Schindelin wrote:
> +   if (!stat(rebase_path_rewritten_list(), ) &&
> +   st.st_size > 0) {
> +   struct child_process child = CHILD_PROCESS_INIT;
> +
> +   child.in = open(rebase_path_rewritten_list(), 
> O_RDONLY);
> +   child.git_cmd = 1;
> +   argv_array_push(, "notes");
> +   argv_array_push(, "copy");
> +   argv_array_push(, "--for-rewrite=rebase");
> +   /* we don't care if this copying failed */
> +   run_command();
> +   }

I know this is a strict port of git-rebase--interactive.sh, but
shouldn't we at least warn the user that the copy failed?

D.

Re: [PATCH 09/22] sequencer: completely revamp the "todo" script parsing

[Johannes Schindelin]

Hi Kuba and Stefan,

On Wed, 31 Aug 2016, Stefan Beller wrote:

> On Wed, Aug 31, 2016 at 10:29 AM, Jakub Narębski  wrote:
> 
> >
> > BTW. perhaps we would be able to continue with 'git continue', regardless
> > of what we have started with, I wonder...
> >
> 
> git continue as a shorthand for `git  --continue` sounds great.

Before we get ahead of ourselves:

1) this has nothing to do with the patch series at hand, and

2) if we were to introduce `git continue`, we would need to think long and
   hard about the following issues:

I) are there potentially ambiguous s that the user
   may want to continue?

II) what about options? You can say `git rebase --continue
--no-ff`, for example, but not `git cherry-pick --continue
--no-ff`...

III) Would it not be confusing to have a subcommand `continue`
 that does *not* serve a *single* purpose? It's kinda flying
 into the face of the Unix philosophy.

Ciao,
Dscho