Re: What's cooking in git.git (Aug 2016, #02; Thu, 4)
On Mon, Aug 08, 2016 at 08:48:55AM +0200, Torsten Bögershausen wrote: > On 2016-08-05 01.32, Junio C Hamano wrote: > > Mike Hommey writes: > [] > > >> What kind of support are you expecting? > > > > The only rationale I recall you justifying this series was that this > > makes the resulting code easier to read, but I do not recall other > > people agreeing with you, and I do not particularly see the > > resulting code easier to follow. > > > If that helps: > I can offer to make a code review, > and come back at the end of the week or so. You already did review that code. Mike -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Aug 2016, #02; Thu, 4)
On 2016-08-05 01.32, Junio C Hamano wrote: > Mike Hommey writes: [] >> What kind of support are you expecting? > > The only rationale I recall you justifying this series was that this > makes the resulting code easier to read, but I do not recall other > people agreeing with you, and I do not particularly see the > resulting code easier to follow. > If that helps: I can offer to make a code review, and come back at the end of the week or so. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from
On Sun, Aug 07, 2016 at 07:02:18PM -1000, Josh Triplett wrote: > > Portability; some compilers choke on it. C89 allows trailing commas in > > array initialization but _not_ in enums. Most compilers allow it anyway > > (though gcc complains with -Wpedantic). > > > > This definitely broke the build on real systems early in Git's history > > (I think the AIX compiler was one culprit), > > Thanks for the explanation. I assume such compilers also don't accept > C99? Correct. We don't allow other C99 features like variadic macros, either (there are some in the code base, but you'll note they can all be conditionally disabled). > Perhaps the next Git user survey could ask "what compiler (including > version) do you use to compile Git", and perhaps "does it accept the > following code:"? Maybe. I'm not sure I would consider a lack of responses there to be a definite sign. It seems that once every few years people on bizarre systems come out of the woodwork and do a round of portability fixes, and then problems accrue, and so on. So I'm not sure that the survey would hit the right people in a timely manner. I think the breaking point will be just declaring "look, C99 is N years old; if your compiler can't handle it, that's now your problem". When Git started, N was only 6. It's now 17. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from
On Mon, Aug 08, 2016 at 12:54:41AM -0400, Jeff King wrote: > On Sun, Aug 07, 2016 at 06:42:07PM -1000, Josh Triplett wrote: > > > > Drop trailing comma after the last enum definition (trailing comma > > > after the last element in an array is OK, though). > > > > I realize this code didn't get included in the final version, but for > > future reference, what's the rationale for this? I tend to include a > > final comma in cases like these (and likewise for initializers) to avoid > > needing to change the last line when introducing a new element, reducing > > noise in diffs. I hadn't seen anything in any of the coding style > > documentation talking about trailing commas (either pro or con). > > Portability; some compilers choke on it. C89 allows trailing commas in > array initialization but _not_ in enums. Most compilers allow it anyway > (though gcc complains with -Wpedantic). > > This definitely broke the build on real systems early in Git's history > (I think the AIX compiler was one culprit), Thanks for the explanation. I assume such compilers also don't accept C99? > but at this point it's > possible that all of those compilers have died off. It would be nice if > we could start using it (for exactly the reasons you give). > Unfortunately there's not a good way to know except "introduce it and > see if people complain". Fair enough. I'll let someone else be the test case for that. :) Perhaps the next Git user survey could ask "what compiler (including version) do you use to compile Git", and perhaps "does it accept the following code:"? - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from
On Sun, Aug 07, 2016 at 06:42:07PM -1000, Josh Triplett wrote: > > Drop trailing comma after the last enum definition (trailing comma > > after the last element in an array is OK, though). > > I realize this code didn't get included in the final version, but for > future reference, what's the rationale for this? I tend to include a > final comma in cases like these (and likewise for initializers) to avoid > needing to change the last line when introducing a new element, reducing > noise in diffs. I hadn't seen anything in any of the coding style > documentation talking about trailing commas (either pro or con). Portability; some compilers choke on it. C89 allows trailing commas in array initialization but _not_ in enums. Most compilers allow it anyway (though gcc complains with -Wpedantic). This definitely broke the build on real systems early in Git's history (I think the AIX compiler was one culprit), but at this point it's possible that all of those compilers have died off. It would be nice if we could start using it (for exactly the reasons you give). Unfortunately there's not a good way to know except "introduce it and see if people complain". -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] format-patch: Add a config option format.from to set the default for --from
On Mon, Aug 01, 2016 at 01:32:49PM -0700, Junio C Hamano wrote:
> Josh Triplett writes:
>
> > +static char *from;
> > static const char *signature = git_version_string;
> > static const char *signature_file;
> > static int config_cover_letter;
> > @@ -807,6 +808,17 @@ static int git_format_config(const char *var, const
> > char *value, void *cb)
> > base_auto = git_config_bool(var, value);
> > return 0;
> > }
> > + if (!strcmp(var, "format.from")) {
> > + int b = git_config_maybe_bool(var, value);
> > + free(from);
> > + if (b < 0)
> > + from = xstrdup(value);
> > + else if (b)
> > + from = xstrdup(git_committer_info(IDENT_NO_DATE));
> > + else
> > + from = NULL;
> > + return 0;
> > + }
>
> One potential issue I see here is that if we ever plan to switch the
> default, we may want to issue a warning message to users who do not
> have any format.from configured when they do run the program on a
> commit that will get a different result before and after the switch
> in a release of Git before that default switch happens. The message
> would say something like "you are formatting somebody else's commit.
> the output will change in future versions of Git and show an explicit
> in-body From: header; if you want to keep the current behaviour, set
> format.from configuration variable to false".
The previous discussion between you and Jeff King seemed to suggest that
a mention in the release notes might suffice, rather than a "noisy
deprecation" warning.
> But you cannot tell by looking at from that is NULL between two
> cases, it is NULL because we defaulted to it (in which case we do
> want to warn), or the user set it explicitly to false (we do not
> want to give the warning).
If we wanted to issue such a warning, I'd suggest a separate boolean
"from_set", set when either the configuration or command line sets
"from", and then the code that handles "from" could emit a warning to
stderr if it would produce different results and !from_set.
- Josh Triplett
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from
On Mon, Aug 01, 2016 at 02:18:47PM -0700, Junio C Hamano wrote:
> Josh Triplett writes:
> > +enum from {
> > + FROM_AUTHOR,
> > + FROM_USER,
> > + FROM_VALUE,
>
> Drop trailing comma after the last enum definition (trailing comma
> after the last element in an array is OK, though).
I realize this code didn't get included in the final version, but for
future reference, what's the rationale for this? I tend to include a
final comma in cases like these (and likewise for initializers) to avoid
needing to change the last line when introducing a new element, reducing
noise in diffs. I hadn't seen anything in any of the coding style
documentation talking about trailing commas (either pro or con).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from
On Sun, Aug 07, 2016 at 06:59:09PM -0700, Junio C Hamano wrote: > Josh Triplett writes: > > > I'd actually seriously considered this exact approach, which I preferred > > as well, but I'd discarded it because I figured it'd get rejected. > > Given your suggestion, and Junio's comment, I'll go with this version. > > Sorry, but your response is soo delayed that I am not sure what you > are agreeing with I'm on vacation right now. I was agreeing with your comment that you didn't care for the change in v2 to use an enum. > and also am not sure if you are planning to reroll > what has already been happily accepted to 'next', which is not quite > welcome. I didn't realize you had already taken the patch series into next; I'd assumed from the various comments that you expected me to reroll it before you'd take it. Would you like me to write something up for the release notes regarding plans to change the default? - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from
Josh Triplett writes: > I'd actually seriously considered this exact approach, which I preferred > as well, but I'd discarded it because I figured it'd get rejected. > Given your suggestion, and Junio's comment, I'll go with this version. Sorry, but your response is soo delayed that I am not sure what you are agreeing with and also am not sure if you are planning to reroll what has already been happily accepted to 'next', which is not quite welcome. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/9] status: collect per-file data for --porcelain=v2
Eric Wong writes:
>> > Not a big deal (no need to resend for this one alone), but let's
>> > make the above properly formatted, i.e.
>> >
>> >if (ce_stage(ce)) {
>> >...
>> >} else {
>> >...
>> >}
>>
>> Do I understand correctly that your objections is against having the curly
>> brace before the "else" on its own line?
>>
>> If so, when did our coding style change? I vividly remember that we
>> strongly favored putting the "else" on a new line after a closing brace,
>> to make diffs nicer in case the braces were removed or added.
>
> AFAIK, Linux kernel CodingStyle has always been what Junio
> suggested (just w/o the trailing spaces :),
> and we inherit from that.
What Eric said.
While I admit that I sometimes break line between "}" and "else {"
by inertia when I am being careless and get caught by checkpatch.pl
myself, I do not recall trying to justify it; you probably may
remember somebody else saying that, but I don't recall anybody
making that argument on the list, and more importantly I don't
recall us making that our style based on that argument.
The only two and half kinds of warnings we knowingly ignore from
scripts/checkpatch.pl in the Linux kernel source tree are:
* "Avoid typedefs." We do avoid making graduitous use of typedef to
hide a structure behind a type and pretty much limit ourselves to
use it for (callback) function types, though.
* "We've never heard of Helped-by/Mentored-by footers"; well,
kernel folks may not, but we have ;-)
* "No spaces for bitfield width". This may not be justifyable, but
the majority of our bitfield widths are defined in the way not
blessed by checkpatch.pl checker.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] archive-tar: make write_extended_header() void
On Sat, Aug 06, 2016 at 04:35:38PM +0200, René Scharfe wrote: > The function write_extended_header() only ever returns 0. Simplify > it and its caller by dropping its return value, like we did with > write_global_extended_header() earlier. Obviously correct, and much nicer to read. Thanks. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] use strbuf_add_unique_abbrev() for adding short hashes
On Sun, Aug 07, 2016 at 10:53:34AM +0200, Johannes Schindelin wrote: > On Sat, 6 Aug 2016, René Scharfe wrote: > > > Call strbuf_add_unique_abbrev() to add abbreviated hashes to strbufs > > instead of taking detours through find_unique_abbrev() and its static > > buffer. This is shorter and a bit more efficient. > > And less error-prone. > > It is also a thing I need to change in my upcoming rebase--helper patches: > I had not known about this function. Great. I added it several months ago to avoid some hairy allocation code, so I am glad to see it finding new uses (both this patch and potential new ones). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from
On Mon, Aug 01, 2016 at 01:38:47PM -0400, Jeff King wrote:
> On Sat, Jul 30, 2016 at 12:11:11PM -0700, Josh Triplett wrote:
>
> > +enum from {
> > + FROM_AUTHOR,
> > + FROM_USER,
> > + FROM_VALUE,
> > +};
> > +
> > +static void set_from(enum from type, const char *value)
> > +{
> > + free(from);
> > + switch (type) {
> > + case FROM_AUTHOR:
> > + from = NULL;
> > + break;
> > + case FROM_USER:
> > + from = xstrdup(git_committer_info(IDENT_NO_DATE));
> > + break;
> > + case FROM_VALUE:
> > + from = xstrdup(value);
> > + break;
> > + }
> > +}
>
> Thanks for looking into reducing the duplication. TBH, I am not sure it
> is really an improvement, just because of the amount of boilerplate (and
> this function interface is kind of weird, because of the rules for when
> "value" should or should not be NULL).
>
> I guess another way to do it would be:
>
> #define FROM_AUTO_IDENT ((const char *)(intptr_t)1))
> void set_from(const char *value)
> {
> if (value == FROM_AUTO_IDENT)
> value = git_committer_info(IDENT_NO_DATE);
> free(from);
> from = xstrdup_or_null(value);
> }
I'd actually seriously considered this exact approach, which I preferred
as well, but I'd discarded it because I figured it'd get rejected.
Given your suggestion, and Junio's comment, I'll go with this version.
- Josh Triplett
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/9] status: collect per-file data for --porcelain=v2
Johannes Schindelin wrote:
> On Fri, 5 Aug 2016, Junio C Hamano wrote:
> > Jeff Hostetler writes:
> > > }
> > > - else
> > > + else {
> > > d->index_status = DIFF_STATUS_ADDED;
> > > + /* Leave {mode,oid}_head zero for adds. */
> > > + d->mode_index = ce->ce_mode;
> > > + hashcpy(d->oid_index.hash, ce->sha1);
> > > + }
> >
> > Not a big deal (no need to resend for this one alone), but let's
> > make the above properly formatted, i.e.
> >
> > if (ce_stage(ce)) {
> > ...
> > } else {
> > ...
> > }
>
> Do I understand correctly that your objections is against having the curly
> brace before the "else" on its own line?
>
> If so, when did our coding style change? I vividly remember that we
> strongly favored putting the "else" on a new line after a closing brace,
> to make diffs nicer in case the braces were removed or added.
AFAIK, Linux kernel CodingStyle has always been what Junio
suggested (just w/o the trailing spaces :),
and we inherit from that.
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/plain/Documentation/CodingStyle
> BTW your suggestion has 24 extra spaces after the final closing brace ;-)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
You there
-- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Forward declaration of enum iterator_selection?
On 05/08/16 23:26, Johannes Sixt wrote: > When refs.c is being compiled, the only mention of enum iterator_selection is > in this piece of code pulled in from refs-internal.h (have a look at the > preprocessed code): > > typedef enum iterator_selection ref_iterator_select_fn( > struct ref_iterator *iter0, struct ref_iterator *iter1, > void *cb_data); > > This looks like a forward declarations of an enumeration type name, something > that I thought is illegal in C. Am I wrong? (That may well be the case, my > C-foo is quite rusty.) At this point 'enum iterator_selection' is an incomplete type and may be used when the size of the object is not required. It is not needed, for example, when a typedef name is being declared as a pointer to, or as a function returning such a type. However, such a type must be complete before such a function is called or defined. > My compiler does not complain (it's gcc 4.8), but I thought I mention it > before someone with a pickier compiler stumbles over it... So, I think this is correct. Having said that, I would rather the 'enum iterator_selection' be defined before this declaration. One solution could be to #include "iterator.h" prior to _all_ #include "refs/refs-internal.h" in all compilation units (Note it is in the opposite order in refs/iterator.c). Alternatively, you could put the #include "../iterator.h" into refs/refs-internal.h directly (some people would object to this). ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] apply: mark some file-local symbols static
Hi Dscho,
On Wed, Aug 3, 2016 at 4:37 PM, Johannes Schindelin
wrote:
> Hi Christian,
>
> On Wed, 3 Aug 2016, Christian Couder wrote:
>
>> Now there are different options to fix this:
>>
>> 1) remove the symbols in 9f87c22 ("apply: refactor `git apply` option
>> parsing") at the end of the series, or
>> 2) move 4820e13 (apply: make some parsing functions static again) at
>> the end of the series and make it also remove them, or:
>> 3) add another patch to remove them after 9f87c22 ("apply: refactor
>> `git apply` option parsing")
>
> You forgot 4) provide fixup patches that fix the patch series.
>
> And 5) fix the patch series, push the branch to GitHub and provide a
> pointer, but not sending a new iteration unless needed otherwise.
Unfortunately there are other changes, especially those suggested by
Peff, I have to make in the patch series, so I will resend everything.
Thanks,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] .mailmap: use Christian Couder's gmail address
Signed-off-by: Christian Couder --- .mailmap | 1 + 1 file changed, 1 insertion(+) diff --git a/.mailmap b/.mailmap index a714e69..a408ac6 100644 --- a/.mailmap +++ b/.mailmap @@ -33,6 +33,7 @@ Cheng Renquan Chris Shoemaker Chris Wright Cord Seele +Christian Couder Christian Stimming Csaba Henk Dan Johnson -- 2.9.2.558.gf53e569 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: patch submission process, was Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors
> On 06 Aug 2016, at 10:58, Johannes Schindelin > wrote: > > Hi Stefan, > > just quickly (i.e. addressing only one point, will try to address more at > a later date) because I want to be mostly offline this weekend: > > On Fri, 5 Aug 2016, Stefan Beller wrote: > >> On Fri, Aug 5, 2016 at 1:20 AM, Johannes Schindelin >> wrote: >>> >>> I also hate to break it to you that git-send-email is not going to be >>> part of any solution. >> >> It's written in perl, so it's not one of the core parts of Git that you >> mentioned above. I do use it though for my submission process. > > The problem is not Perl, but how fiddly it is to set up. And that you lose > all the niceties of an email client (e.g. when you want to add a comment > before the diff stat that is not intended to become a part of the commit > message). > > But I had an apostrophe last night. I might have been a bit overzealous to > claim that git-send-email won't be a part of the solution. It cannot be > a *user-visible* part of any solution, that still holds true. > > So here is the apostrophe: why not implement a bot that listens to the PRs > on GitHub, and accepts commands such as "@bot please send this > upstream" via comments on the PR. It would then send the patches to the > list, from its own email address, on behalf of the contributor. > > Lots of things to kink out, such as: does it need to be moderated? Record > what was submitted in its own git.git fork? Accept replies and attach them > to the correct PR? Maybe annotate those replies with the commits whose > patches were discussed? Maybe send out replies on the PR as emails? Maybe > try to auto-apply suggested patches? Cc: people who commented on earlier > iterations of the patch series? Maybe make interaction smarter using an AI > bot framework? > > If only I had lots of time on my hand, I'd start by prototyping a node.js > server and hooking it up via webhooks, then show it off so others can > tinker with it. > > This is not completely unlike submitGit, which was a good first attempt, > but I never used it because I needed it to do so much more than it already > did, *and* it complicated everything by requiring users to register with > an extra step to allow submitGit to send email on their behalf. It also > made contributing to it harder by choosing some non-standard web app > framework. Also, I really do not like having to go to a different website > just to send a GitHub PR to the list. > > Anyway, that was my brain fart for the day. Great discussion! I would like to share my perspective a someone who is a (relatively speaking) new Git contributor and who has never interacted on mailing lists before Git: 1.) "git format-patch" and "git send-email" work great for me. It took some time to learn how they work but now I have my own "submit.sh" based on those tools and posting a new series is a piece of cake. 2.) Initially it was hard for me to ensure that my patches don't break build or tests on Linux and OS X. Travis CI helps me a lot. I just wished we could get Windows support, too. 3.) I noticed that I get two types of reviews. The first kind points out things in my patch that are obviously wrong. The second kind are things that require a discussion. When I get feedback of the first kind, then I am always super eager to send out a new roll just because I don't want any other reviewer to waste time on obviously wrong patches. However, I have the impression that frequent re-rolls are frowned upon. If we would use Git for the patches instead of email, then I could add "squash" patches to indicate changes in the current roll that will be squashed in the next roll (I know I could send squash patches as email, too... but for me that gets confusing quickly). 4.) Reviewing patches is super hard for me because my email client does not support patch color highlighting and I can't easily expand context or look at the history of code touched by the patch (e.g via git blame). I tried to setup Alpine but I wasn't happy with the interface either. I like patches with a GitHub URL for review but then I need to find the right line in the original email to write a comment. Again, this is just my point of view as a "newbie" and I definitively don't expect the Git community to change their established workflows. Cheers, Lars-- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: storing cover letter of a patch series?
On Sun, Aug 7, 2016 at 7:12 AM, Michael S. Tsirkin wrote: > On Fri, Aug 05, 2016 at 08:39:58AM -0700, Junio C Hamano wrote: >> "Michael S. Tsirkin" writes: >> >> > On Thu, Sep 10, 2015 at 11:39:49AM -0700, Junio C Hamano wrote: >> >> The problem with "empty commit trick" is that it is a commit whose >> >> sole purpose is to describe the series, and its presence makes it >> >> clear where the series ends, but the topology does not tell where >> >> the series begins, so it is an unsatisifactory half-measure. >> > >> > Actually, when using topic branches the series always ends at head, so >> > it's better to keep the empty commit where series begins. >> >> But that would mean that you would need to destroy and recreate more >> commits than you would need to. If you have a five-commit series >> (with the bottom "description" one, you would have six commits) and >> you are already happy with the bottom two but want to update the >> third one, you wuld have to "rebase -i" all six of them, reword the >> bottom "description" to adjust it to describe the new version of the >> third one _before_ you even do the actual update of the third one. >> >> That somehow feels backwards, and that backward-ness comes from the >> fact that you abused a single-parent commit for the purpose it is >> not meant to be used (i.e. they are to describe individual changes), >> because you did not find a better existing mechanism (and I suspect >> there isn't any, in which case the solution is to invent one, not >> abusing an existing mechanism that is not suited for it). > > A flag that marks a commit "beginning of series" then? git-notes was mentioned in this thread back in 2015, but I think it's discarded because of the argument that's part of the cover letter was not meant to be kept permanently. But I think we can still use it as a local/temporary place for cover letter instead of the empty commit at the topic's tip. It is a mark of the beginning of commit, it does not require rewriting history when you update the cover letter, and git-merge can be taught to pick it up when you're ready to set it in stone. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: storing cover letter of a patch series?
On Sun, Aug 07, 2016 at 08:12:23AM +0300, Michael S. Tsirkin wrote: > On Fri, Aug 05, 2016 at 08:39:58AM -0700, Junio C Hamano wrote: > > * When you updated an existing topic, you tell a tool like "rebase > >-i -p" to recreate "lit" branch on top of the mainline. This > >would give you an opportunity to update the cover. > > Combining patchsets might need conflict resolution, > redoing this each time might be a lot of work. git-rerere can generally handle that pretty well. I wrote a tool [1] to manage integration branches which I use pretty heavily and I find it very rare to hit a serious conflict. In fact, git-integration has an "autocontinue" mode which accepts git-rerere's resolution if it has one, which works reliably in my experience. I hadn't thought about writing the cover letter in the integration branch instruction sheet (I normally just put in some notes for myself about the state of the branch), but I suspect it would be quite easy to write a script that mails a series using the instruction sheet comments as the cover letter. [1] http://johnkeeping.github.io/git-integration/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Rename detection within in files WAS: [PATCH 2/6] t7408: merge short tests, factor out testing method
Am 06.08.2016 um 01:26 schrieb Stefan Beller:
When moving code around, we usually get large chunks of text. If the contributor
is not 100% trustworthy, we need to review all the code without much intelectual
joy. Essentially the reviewer is just making sure the parts of the text are the
same.
I'd like to propose a new addition to the diff format that makes this use case
easier. The idea is to mark up lines that were just moved around in the file
instead of adding and removing them.
Currently we have 3 characters that
are allowed to start a line within a hunk:
' ' to indicate context
'+' to add a line
'-' to remove a line
I'd propose to add the following characters:
'*' which is the same as '+', but it indicates that the line was moved
from somewhere else without change.
'X' The same as '-', with the addition that this line was moved to a different
place without change.
The patch below uses these new '*' and 'X'. Each hunk that makes use of these
additions, is followed other sections, [moved-from, moved-to] that indicate
where the corresponding line is.
Interesting idea. It should be easy to convert the result into a regular
unified diff for consumption with patch(1) or git am/apply by replacing
the new flags with + and - and removing the moved-* hunks.
Your example ignores whitespace changes at the start of the line and
within it, the added "-C $working_dir", s/expected/expect/; is this all
intended? Only a single blank line was moved verbatim.
The moved-from and moved-to hunks make this diff quite verbose.
If multiple lines from different sources are moved to the same hunk then
you'd get multiple moved-from hunks following that single destination,
right? (Same with lines moved from a single hunk to multiple
destinations and moved-to.)
But does it even warrent a new format? It's a display problem; the
necessary information is already in the diffs we have today. A
graphical diff viewer could connect moved blocks with lines, like
http://www.araxis.com/merge/ does in its side-by-side view. A
Thunderbird extension (or a bookmarklet or browser extendiion for
webmail users) could do that for an email-based workflow.
Still, what about adding information about moved lines as an extended
header (like that index line)? Line numbers are included in hunk
headers and can serve as orientation. A reader would have to do some
mental arithmetic (ugh), but incompatible format changes would be
avoided. For your example it should look something like this:
move from t/t7408-submodule-reference.sh:52,1
move to t/t7408-submodule-reference.sh:22,1
There are multiple things to tackle when going for such an addition:
* How to present this to the user (it's covered in this email)
* how to find the renamed lines algorithmically.
(there are already approaches to that, e.g.
https://github.com/stefanbeller/duplo
which is http://duplo.sourceforge.net/ with no substantial additions)
Any comments welcome,
Thanks,
Stefan
---
t/t7408-submodule-reference.sh | 50 +++---
1 file changed, 15 insertions(+), 29 deletions(-), 6 moved lines
diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
index afcc629..1416cbd 100755
--- a/t/t7408-submodule-reference.sh
+++ b/t/t7408-submodule-reference.sh
@@ -10,6 +10,16 @@ base_dir=$(pwd)
U=$base_dir/UPLOAD_LOG
+test_alternate_usage()
+{
+ alternates_file=$1
+ working_dir=$2
+ test_line_count = 1 $alternates_file &&
* echo "0 objects, 0 kilobytes" >expect &&
* git -C $working_dir count-objects >current &&
* diff expect current
+}
+
Post-image line 22.
test_expect_success 'preparing first repository' '
test_create_repo A &&
(
@@ move-source 42,6 @@ test_expect_success 'that reference gets used with add' '
test_expect_success 'that reference gets used with add' '
(
cd super/sub &&
X echo "0 objects, 0 kilobytes" > expected &&
X git count-objects > current &&
X diff expected current
)
'
@@ -42,44 +52,20 @@ test_expect_success 'preparing superproject' '
)
'
-test_expect_success 'submodule add --reference' '
+test_expect_success 'submodule add --reference uses alternates' '
(
cd super &&
git submodule add --reference ../B "file://$base_dir/A" sub &&
git commit -m B-super-added
- )
-'
-
Pre-image line 52.
-test_expect_success 'after add: existence of info/alternates' '
- test_line_count = 1 super/.git/modules/sub/objects/info/alternates
-'
-
-test_expect_success 'that reference gets used with add' '
- (
- cd super/sub &&
X echo "0 objects, 0 kilobytes" > expected &&
X git count-objects > current &&
X diff expected current
- )
-'
-
-test_expect_success 'cloning superproject' '
- git
Re: What's cooking in git.git (Aug 2016, #02; Thu, 4)
Hi Junio, On Thu, 4 Aug 2016, Junio C Hamano wrote: > * js/import-tars-hardlinks (2016-08-03) 1 commit > - import-tars: support hard links > > "import-tars" fast-import script (in contrib/) used to ignore a > hardlink target and replaced it with an empty file, which has been > corrected to record the same blob as the other file the hardlink is > shared with. For the record: this came up, and is required, for my CI work on Windows, as many MSYS2 packages contain hard-linked files (think about *all* the builtins in Git, for example). I will use Git to synchronize MSYS2 setups between build agents. Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] use strbuf_add_unique_abbrev() for adding short hashes
Hi René, On Sat, 6 Aug 2016, René Scharfe wrote: > Call strbuf_add_unique_abbrev() to add abbreviated hashes to strbufs > instead of taking detours through find_unique_abbrev() and its static > buffer. This is shorter and a bit more efficient. And less error-prone. It is also a thing I need to change in my upcoming rebase--helper patches: I had not known about this function. Thank you, Dscho
Re: [PATCH] merge-recursive: use STRING_LIST_INIT_NODUP
Hi René, On Fri, 5 Aug 2016, René Scharfe wrote: > Initialize a string_list right when it's defined. That's shorter, saves > a function call and makes it more obvious that we're using the NODUP > variant here. Thank you! I guess I never updated the code after the _INIT* macros were introduced. The change is good, of course. Thanks, Dscho
Re: [PATCH] use strbuf_addstr() instead of strbuf_addf() with "%s"
Hi René, On Fri, 5 Aug 2016, René Scharfe wrote: > Call strbuf_addstr() for adding a simple string to a strbuf instead of > using the heavier strbuf_addf(). This is shorter and documents the > intent more clearly. Looks obviously good, thanks! Ciao, Dscho
Re: patch submission process, was Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors
Hi, On Sat, 6 Aug 2016, Eric Wong wrote: > Junio C Hamano wrote: > > Somebody mentioned "configuring it is hard--why does the user have > > to know SMTP subtleties", and that may be a valid complaint against > > the primary part of send-email. The solution for that is not to > > discard it with bathwater, but make it just as easy as other MSAs, > > say, Thunderbird, to configure for an average user who can configure > > these other MUAs. > > Sadly, the average user does not use an MUA, SMTP or IMAP, anymore. > It's all webmail or apps using proprietary protocols. > Embrace, extend, extinguish :< I think you both got it wrong. The original citizens were the mail clients that allowed you to communicate with other human beings. Webmail is just a new generation of the same commodity. It is our usage to transport machine-readable content (and not as an attachment!) that is the intruder. It's not making things better if we require users to use a second mail client for sending out patches, and, oh, it does nothing to help with reintegrating patches back into Git, were they had been before taking that perilous and lossy journey through that medium called email. Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/9] status: collect per-file data for --porcelain=v2
Hi Junio,
On Fri, 5 Aug 2016, Junio C Hamano wrote:
> Jeff Hostetler writes:
>
> > if (ce_stage(ce)) {
> > d->index_status = DIFF_STATUS_UNMERGED;
> > d->stagemask |= (1 << (ce_stage(ce) - 1));
> > + /*
> > +* Don't bother setting {mode,oid}_{head,index} since
> > the print
> > +* code will output the stage values directly and not
> > use the
> > +* values in these fields.
> > +*/
> > }
> > - else
> > + else {
> > d->index_status = DIFF_STATUS_ADDED;
> > + /* Leave {mode,oid}_head zero for adds. */
> > + d->mode_index = ce->ce_mode;
> > + hashcpy(d->oid_index.hash, ce->sha1);
> > + }
>
> Not a big deal (no need to resend for this one alone), but let's
> make the above properly formatted, i.e.
>
> if (ce_stage(ce)) {
> ...
> } else {
> ...
> }
Do I understand correctly that your objections is against having the curly
brace before the "else" on its own line?
If so, when did our coding style change? I vividly remember that we
strongly favored putting the "else" on a new line after a closing brace,
to make diffs nicer in case the braces were removed or added.
BTW your suggestion has 24 extra spaces after the final closing brace ;-)
Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
