[PATCH] Avoid the use of backslash-at-eol in pack-objects usage string.

[PATCH] Avoid the use of backslash-at-eol in pack-objects usage string.

[Thiago Farina]

Signed-off-by: Thiago Farina <tfransosi@gmail.com>
---
 builtin-pack-objects.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 7a390e1..4494a68 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -22,15 +22,15 @@
 #include <pthread.h>
 #endif
 
-static const char pack_usage[] = "\
-git pack-objects [{ -q | --progress | --all-progress }] \n\
-	[--max-pack-size=N] [--local] [--incremental] \n\
-	[--window=N] [--window-memory=N] [--depth=N] \n\
-	[--no-reuse-delta] [--no-reuse-object] [--delta-base-offset] \n\
-	[--threads=N] [--non-empty] [--revs [--unpacked | --all]*] [--reflog] \n\
-	[--stdout | base-name] [--include-tag] \n\
-	[--keep-unreachable | --unpack-unreachable] \n\
-	[<ref-list | <object-list]";
+static const char pack_usage[] =
+  "git pack-objects [{ -q | --progress | --all-progress }] \n"
+  "        [--max-pack-size=N] [--local] [--incremental] \n"
+  "        [--window=N] [--window-memory=N] [--depth=N] \n"
+  "        [--no-reuse-delta] [--no-reuse-object] [--delta-base-offset] \n"
+  "        [--threads=N] [--non-empty] [--revs [--unpacked | --all]*] [--reflog] \n"
+  "        [--stdout | base-name] [--include-tag] \n"
+  "        [--keep-unreachable | --unpack-unreachable] \n"
+  "        [<ref-list | <object-list]";
 
 struct object_entry {
 	struct pack_idx_entry idx;
-- 
1.6.5.rc0.dirty

Re: [PATCH] Avoid the use of backslash-at-eol in pack-objects usage string.

[Junio C Hamano]

Thiago Farina <tfransosi@gmail.com> writes:

> +static const char pack_usage[] =
> +  "git pack-objects [{ -q | --progress | --all-progress }] \n"
> +  "        [--max-pack-size=N] [--local] [--incremental] \n"
> +  "        [--window=N] [--window-memory=N] [--depth=N] \n"
> +  "        [--no-reuse-delta] [--no-reuse-object] [--delta-base-offset] \n"
> +  "        [--threads=N] [--non-empty] [--revs [--unpacked | --all]*] [--reflog] \n"
> +  "        [--stdout | base-name] [--include-tag] \n"
> +  "        [--keep-unreachable | --unpack-unreachable] \n"
> +  "        [<ref-list | <object-list]";

Do you still want to keep the trailing whitespace on these lines?

Re: [PATCH] Avoid the use of backslash-at-eol in pack-objects usage string.

[Thiago Farina]

On Thu, Sep 17, 2009 at 7:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Thiago Farina <tfransosi@gmail.com> writes:
>
>> +static const char pack_usage[] =
>> +  "git pack-objects [{ -q | --progress | --all-progress }] \n"
>> +  "        [--max-pack-size=N] [--local] [--incremental] \n"
>> +  "        [--window=N] [--window-memory=N] [--depth=N] \n"
>> +  "        [--no-reuse-delta] [--no-reuse-object] [--delta-base-offset] \n"
>> +  "        [--threads=N] [--non-empty] [--revs [--unpacked | --all]*] [--reflog] \n"
>> +  "        [--stdout | base-name] [--include-tag] \n"
>> +  "        [--keep-unreachable | --unpack-unreachable] \n"
>> +  "        [<ref-list | <object-list]";
>
> Do you still want to keep the trailing whitespace on these lines?
I did this to maintain the same output of the old string, but if you
want I can change, what you suggest?

Re: [PATCH] Avoid the use of backslash-at-eol in pack-objects usage string.

[Junio C Hamano]

Thiago Farina <tfransosi@gmail.com> writes:

> On Thu, Sep 17, 2009 at 7:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Thiago Farina <tfransosi@gmail.com> writes:
>>
>>> +static const char pack_usage[] =
>>> +  "git pack-objects [{ -q | --progress | --all-progress }] \n"
>>> +  "        [--max-pack-size=N] [--local] [--incremental] \n"
>>> +  "        [--window=N] [--window-memory=N] [--depth=N] \n"
>>> +  "        [--no-reuse-delta] [--no-reuse-object] [--delta-base-offset] \n"
>>> +  "        [--threads=N] [--non-empty] [--revs [--unpacked | --all]*] [--reflog] \n"
>>> +  "        [--stdout | base-name] [--include-tag] \n"
>>> +  "        [--keep-unreachable | --unpack-unreachable] \n"
>>> +  "        [<ref-list | <object-list]";
>>
>> Do you still want to keep the trailing whitespace on these lines?
> I did this to maintain the same output of the old string, but if you
> want I can change, what you suggest?

If you need to add or remove an option to actually _change_ the string, a
patch like this, as a preparatory step before the real improvement, would
be a very welcome clean-up.  I however would suggest doing nothing, if
this is the only patch you are going to send against this program in the
near future, to be honest.

Even though we do not have any other patch in flight that changes this
program at this moment (as expected, because we are in -rc freeze), which
means there is not much risk for this patch to cause needless conflicts
with others, we generally avoid code churn like this one, as a principle
for a maturing project.

The _very best_ thing you can do for the project on this particular issue
is to keep an eye on the list and the next time somebody wants to patch
this program in a way that affects the usage string, remind that person to
first clean-up the string without changing anything else as a preparation
patch; I however admit that I am asking a lot more work out of you.

A real improvement patch from that somebody _could_ be to remove the
trailing whitespaces from the output string, and in that case I would not
mind if two patches (one preparatory patch which is this one, and the
other being the removal of trailing whitespaces) were squashed together.
In fact, in such a trivial case, it probably be better to squash them into
one.

And that somebody _could_ be you.

Re: [PATCH] Avoid the use of backslash-at-eol in pack-objects usage string.

[Thiago Farina]

On Thu, Sep 17, 2009 at 9:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Thiago Farina <tfransosi@gmail.com> writes:
>
>> On Thu, Sep 17, 2009 at 7:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Thiago Farina <tfransosi@gmail.com> writes:
>>>
>>>> +static const char pack_usage[] =
>>>> +  "git pack-objects [{ -q | --progress | --all-progress }] \n"
>>>> +  "        [--max-pack-size=N] [--local] [--incremental] \n"
>>>> +  "        [--window=N] [--window-memory=N] [--depth=N] \n"
>>>> +  "        [--no-reuse-delta] [--no-reuse-object] [--delta-base-offset] \n"
>>>> +  "        [--threads=N] [--non-empty] [--revs [--unpacked | --all]*] [--reflog] \n"
>>>> +  "        [--stdout | base-name] [--include-tag] \n"
>>>> +  "        [--keep-unreachable | --unpack-unreachable] \n"
>>>> +  "        [<ref-list | <object-list]";
>>>
>>> Do you still want to keep the trailing whitespace on these lines?
>> I did this to maintain the same output of the old string, but if you
>> want I can change, what you suggest?
>
> If you need to add or remove an option to actually _change_ the string, a
> patch like this, as a preparatory step before the real improvement, would
> be a very welcome clean-up.  I however would suggest doing nothing, if
> this is the only patch you are going to send against this program in the
> near future, to be honest.
OK.
>
> Even though we do not have any other patch in flight that changes this
> program at this moment (as expected, because we are in -rc freeze), which
> means there is not much risk for this patch to cause needless conflicts
> with others, we generally avoid code churn like this one, as a principle
> for a maturing project.
This release candidate freeze is a period that no one can send patches?
If you could, point me to a documentation about how is the development
process adopted by git.
As I can see, anybody can send patches to this mailing list for
review, but if no one cares about the my patch for example, it doesn't
get a review and any feedback.
In a web review tool, the patch is assigned to someone review it, but
here it is impossible, so how the things are tracked here? Only you
merge the patches into the master branch? When someone send a patch,
you get it, make a topic-branch in your private repository, and if it
is good it will be merged into 'pu branch'? And what did you mean with
code churn?
>
> The _very best_ thing you can do for the project on this particular issue
> is to keep an eye on the list and the next time somebody wants to patch
> this program in a way that affects the usage string, remind that person to
> first clean-up the string without changing anything else as a preparation
> patch; I however admit that I am asking a lot more work out of you.
>
> A real improvement patch from that somebody _could_ be to remove the
> trailing whitespaces from the output string, and in that case I would not
> mind if two patches (one preparatory patch which is this one, and the
> other being the removal of trailing whitespaces) were squashed together.
> In fact, in such a trivial case, it probably be better to squash them into
> one.
If I understand correctly, do you want a function to remove the
trailing whitespace from a given string? Like the functions that work
with whitespaces in ws.c?
>
> And that somebody _could_ be you.
>

Re: [PATCH] Avoid the use of backslash-at-eol in pack-objects usage string.

[Junio C Hamano]

Thiago Farina <tfransosi@gmail.com> writes:

> This release candidate freeze is a period that no one can send patches?

No.

After -rc1, only fixes to regressions and severe bugs and trivially
correct documentation patches will be applied to my tree, but all other
kinds of patches are still sent to the list for discussion, so that the
proposed changes can be discussed, polished and then become ready for the
development cycle after the upcoming release.  I often even pick them up
and queue them to 'pu' and possibly 'next' as time permits.

> Only you
> merge the patches into the master branch? When someone send a patch,
> you get it, make a topic-branch in your private repository, and if it
> is good it will be merged into 'pu branch'?

"A note from the maintainer" I send out every once in a while (also can be
seen at http://members.cox.net/junkio/) explains how things work.

> And what did you mean with code churn?

A change primarily for the sake of change without urgency nor real benefit
in the longer term.

It bothers nobody if a long literal string is written as a string literal
in a dq pair with LFs quoted with backslashes, or as a run of multiple
string literals, each of which ending with LF, to be concatenated by the
compiler.  It however would bother somebody who actually wants to modify
these lines for a real change, and that is the best time for doing such a
clean-up.  Reasons for such a real change vary; to fix earlier mistakes
(e.g. one line being excessively longer than others, or an option is
misspelled), to add a new option, or to make the output of the program
easier to read in general, etc.

>> A real improvement patch from that somebody _could_ be to remove the
>> trailing whitespaces from the output string, and in that case I would not
>> mind if two patches (one preparatory patch which is this one, and the
>> other being the removal of trailing whitespaces) were squashed together.
>> In fact, in such a trivial case, it probably be better to squash them into
>> one.

> If I understand correctly, do you want a function...

No.

What I meant was that I might have said it is a real improvement if your
patch also removed the trailing whitespace from the literal string, as I
hinted in my original response.

Such a submission may have looked like this.  Notice that the changing of
the style for multi-line string literal is "while at it".  I called a
patch whose only change falls into that category a needless code churn.

-- >8 --
Subject: [PATCH] pack-objects: remove SP at the end of usage string
From: Thiago Farina <tfransosi@gmail.com>

These spaces immediately before the end of lines are unnecessary.

While at it, instead of using a single string literal with backslashes
at end of each line, split the lines into individual string literals
and tell the compiler to concatenate them.

Signed-off-by: Thiago Farina <tfransosi@gmail.com>
---
 builtin-pack-objects.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 7a390e1..02f9246 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -22,15 +22,15 @@
 #include <pthread.h>
 #endif
 
-static const char pack_usage[] = "\
-git pack-objects [{ -q | --progress | --all-progress }] \n\
-	[--max-pack-size=N] [--local] [--incremental] \n\
-	[--window=N] [--window-memory=N] [--depth=N] \n\
-	[--no-reuse-delta] [--no-reuse-object] [--delta-base-offset] \n\
-	[--threads=N] [--non-empty] [--revs [--unpacked | --all]*] [--reflog] \n\
-	[--stdout | base-name] [--include-tag] \n\
-	[--keep-unreachable | --unpack-unreachable] \n\
-	[<ref-list | <object-list]";
+static const char pack_usage[] =
+  "git pack-objects [{ -q | --progress | --all-progress }]\n"
+  "        [--max-pack-size=N] [--local] [--incremental]\n"
+  "        [--window=N] [--window-memory=N] [--depth=N]\n"
+  "        [--no-reuse-delta] [--no-reuse-object] [--delta-base-offset]\n"
+  "        [--threads=N] [--non-empty] [--revs [--unpacked | --all]*]\n"
+  "        [--reflog] [--stdout | base-name] [--include-tag]\n"
+  "        [--keep-unreachable | --unpack-unreachable \n"
+  "        [<ref-list | <object-list]";
 
 struct object_entry {
 	struct pack_idx_entry idx;

Re: [PATCH] Avoid the use of backslash-at-eol in pack-objects usage string.

[Thiago Farina]

On Fri, Sep 18, 2009 at 4:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Thiago Farina <tfransosi@gmail.com> writes:
>
>> This release candidate freeze is a period that no one can send patches?
>
> No.
>
> After -rc1, only fixes to regressions and severe bugs and trivially
> correct documentation patches will be applied to my tree, but all other
> kinds of patches are still sent to the list for discussion, so that the
> proposed changes can be discussed, polished and then become ready for the
> development cycle after the upcoming release.
> I often even pick them up and queue them to 'pu' and possibly 'next' as time permits.
If you don't want to pick a patch this means that it was not accepted,
right? I wrote others two trivial patches, one has comments, and I did
the changes suggested, but I guess not will be done about it because
it is just trivial.
>> And what did you mean with code churn?
>
> A change primarily for the sake of change without urgency nor real benefit
> in the longer term.
>
> It bothers nobody if a long literal string is written as a string literal
> in a dq pair with LFs quoted with backslashes, or as a run of multiple
> string literals, each of which ending with LF, to be concatenated by the
> compiler.  It however would bother somebody who actually wants to modify
> these lines for a real change, and that is the best time for doing such a
> clean-up.  Reasons for such a real change vary; to fix earlier mistakes
> (e.g. one line being excessively longer than others, or an option is
> misspelled), to add a new option, or to make the output of the program
> easier to read in general, etc.
This means that trivial patches like this one I wrote are generally
not accepted? Why is there this difficult (is it to maintain the high
level of the patches)? I thought if it is trivial it can be merged
after a review, into one of the integration branches. You write the
comments (the people in mailing list), I make the changes, and then
the patch is committed. But what I'm seeing here, this is not how the
things are done here. It is much more complicated than that I guess.

In a codereview tool I can send a patch for review, I can assign it to
someone review, he will make comments, I will make the necessary
changes, and when the patch is ready, it will be committed. What is
the workflow? With an email I can't assign a patch to someone, with
time it will be lost.

I'm just trying to understand what I have to do, to submit better
patches. Another issue that I saw, is about *issues* or bugs, they are
not tracked in a bug traker. It's just an email, so how can I work in
a bug if I don't know about it, have I to find the bugs myself?