Re: [PATCH v4 01/10] ref-filter: add option to align atoms to the left
On Mon, Jul 27, 2015 at 5:18 PM, Duy Nguyen pclo...@gmail.com wrote:
On Mon, Jul 27, 2015 at 2:39 PM, Jacob Keller jacob.kel...@gmail.com wrote:
On Sun, Jul 26, 2015 at 5:39 PM, Duy Nguyen pclo...@gmail.com wrote:
On Sun, Jul 26, 2015 at 11:08 AM, Eric Sunshine sunsh...@sunshineco.com
wrote:
You can generate an interdiff with git diff branchname-v4
branchname-v5, for instance.
Off topic. But what stops me from doing this often is it creates a big
mess in git tag -l. Do we have an option to hide away some
insignificant: tags? reflog might be an option if we have something
like foo@{/v2} to quickly retrieve the reflog entry whose message
contains v2.
...
But maybe we're abusing reflog..
Actually a good place for this stuff is git branch
--edit-description. A lot of manual steps to save old refs, do
inter-diff.. but it's probably good enough.
--
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: [PATCH v4 01/10] ref-filter: add option to align atoms to the left
Karthik Nayak karthik@gmail.com writes: I've been working on the same branch, and that's why I didn't really provide interdiff's, You can keep working on the same branch and tag versions you send to the list. This state is what I sent to the list as vX is something that does not change in time hence a tag avoids mistakenly changing it. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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 v4 01/10] ref-filter: add option to align atoms to the left
On Mon, Jul 27, 2015 at 2:39 PM, Jacob Keller jacob.kel...@gmail.com wrote:
On Sun, Jul 26, 2015 at 5:39 PM, Duy Nguyen pclo...@gmail.com wrote:
On Sun, Jul 26, 2015 at 11:08 AM, Eric Sunshine sunsh...@sunshineco.com
wrote:
You can generate an interdiff with git diff branchname-v4
branchname-v5, for instance.
Off topic. But what stops me from doing this often is it creates a big
mess in git tag -l. Do we have an option to hide away some
insignificant: tags? reflog might be an option if we have something
like foo@{/v2} to quickly retrieve the reflog entry whose message
contains v2.
You can normally find the previous commit via the reflog. Various
changes to the settings can make the reflog be maintained for longer
if you have longer lived patch series. That's how I would suggest it,
rather than branches, as I tend not to keep old versions around on
separate tags or branches.
The problem with foo@{/v2} is that people don't always keep values
inside the message it self, but maybe foo@{/pattern} would be a
useful extension?
reflog message is different from commit message. I was referring
to the first one (which is out of user control), perhaps you were
referring to the second one? I don't expect people to add v2 to
manually. If we have something like foo@{/v2} then we can teach
send-email (or format-patch) to add a reflog entry with v2 in it.
But maybe we're abusing reflog..
--
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: [PATCH v4 01/10] ref-filter: add option to align atoms to the left
On Sun, Jul 26, 2015 at 5:39 PM, Duy Nguyen pclo...@gmail.com wrote:
On Sun, Jul 26, 2015 at 11:08 AM, Eric Sunshine sunsh...@sunshineco.com
wrote:
You can generate an interdiff with git diff branchname-v4
branchname-v5, for instance.
Off topic. But what stops me from doing this often is it creates a big
mess in git tag -l. Do we have an option to hide away some
insignificant: tags? reflog might be an option if we have something
like foo@{/v2} to quickly retrieve the reflog entry whose message
contains v2.
--
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
You can normally find the previous commit via the reflog. Various
changes to the settings can make the reflog be maintained for longer
if you have longer lived patch series. That's how I would suggest it,
rather than branches, as I tend not to keep old versions around on
separate tags or branches.
The problem with foo@{/v2} is that people don't always keep values
inside the message it self, but maybe foo@{/pattern} would be a
useful extension?
Regards,
Jake
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 01/10] ref-filter: add option to align atoms to the left
On Sun, Jul 26, 2015 at 11:08 AM, Eric Sunshine sunsh...@sunshineco.com wrote:
You can generate an interdiff with git diff branchname-v4
branchname-v5, for instance.
Off topic. But what stops me from doing this often is it creates a big
mess in git tag -l. Do we have an option to hide away some
insignificant: tags? reflog might be an option if we have something
like foo@{/v2} to quickly retrieve the reflog entry whose message
contains v2.
--
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: [PATCH v4 01/10] ref-filter: add option to align atoms to the left
On Sun, Jul 26, 2015 at 11:28 AM, Eric Sunshine sunsh...@sunshineco.com wrote: On Sun, Jul 26, 2015 at 12:36 AM, Karthik Nayak karthik@gmail.com wrote: On Sun, Jul 26, 2015 at 9:38 AM, Eric Sunshine sunsh...@sunshineco.com wrote: Also, it is helpful to reviewers if you include an interdiff at the bottom of your cover letter showing the changes from one version to another. You can generate an interdiff with git diff branchname-v4 branchname-v5, for instance. I've been working on the same branch, and that's why I didn't really provide interdiff's, and I kinda worked on the same branch again, so I wont be giving an interdiff for the next series either, but I'll keep this in mind and follow it from the forthcoming patch series. Thanks You can still provide an interdiff. It is quite likely that you can find the commit corresponding to v4 in the reflog for that branch (see git-reflog). Failing that, an easy way to recover the state at v4 is to create a new branch (from the parent of your current working branch) and apply the v4 patches which you sent to the mailing list. If Junio queued v4 in his 'pu' branch, then you can also recover from there. Once you've recovered the state of v4 using one of the methods mentioned here (or some other), you can make an interdiff when sending out v5. Haha, I was just thinking about applying my patches and doing it, will definitely provide an interdiff. Reviewers do appreciate that you provide a URL to the previous version and take the time to explain in your cover letter what changed (and why). Including an interdiff is one more way to ease the review process, and is also appreciated, so it may be worthwhile to put in the effort to recover the state of v4 so that you can include an interdiff with v5. Doing so does require a bit of extra work for you, but that's work you only need to do *once*, whereas if you don't do it, then you place the burden on *each* reviewer for *each* version, which quickly adds up to a lot more work for those reviewing your submissions. Yes! definitely, I'll make sure that I provide the interdiff, I'll look at reflog, thanks a lot. I appreciate how reviewers take time to review code, its a wonderful process. I will be glad to put in any time to make their process easier -- Regards, Karthik Nayak -- 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 v4 01/10] ref-filter: add option to align atoms to the left
On Fri, Jul 24, 2015 at 3:04 PM, Karthik Nayak karthik@gmail.com wrote: Add a new atom align and support %(align:X) where X is a number. This will align the preceeding atom value to the left followed by Do you mean succeeding or following or next (or something) rather than preceding? spaces for a total length of X characters. If X is less than the item size, the entire atom value is printed. Isn't this a pad-right operation? If so, should this be called %(padright:X) or %(pad:right:X)? Signed-off-by: Karthik Nayak karthik@gmail.com Also, it is helpful to reviewers if you include an interdiff at the bottom of your cover letter showing the changes from one version to another. You can generate an interdiff with git diff branchname-v4 branchname-v5, for instance. -- 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 v4 01/10] ref-filter: add option to align atoms to the left
On Sun, Jul 26, 2015 at 9:38 AM, Eric Sunshine sunsh...@sunshineco.com wrote: On Fri, Jul 24, 2015 at 3:04 PM, Karthik Nayak karthik@gmail.com wrote: Add a new atom align and support %(align:X) where X is a number. This will align the preceeding atom value to the left followed by Do you mean succeeding or following or next (or something) rather than preceding? I meant succeeding, I had just changed that, thanks for telling spaces for a total length of X characters. If X is less than the item size, the entire atom value is printed. Isn't this a pad-right operation? If so, should this be called %(padright:X) or %(pad:right:X)? I guess padright makes more sense, thanks. Signed-off-by: Karthik Nayak karthik@gmail.com Also, it is helpful to reviewers if you include an interdiff at the bottom of your cover letter showing the changes from one version to another. You can generate an interdiff with git diff branchname-v4 branchname-v5, for instance. I've been working on the same branch, and that's why I didn't really provide interdiff's, and I kinda worked on the same branch again, so I wont be giving an interdiff for the next series either, but I'll keep this in mind and follow it from the forthcoming patch series. Thanks -- Regards, Karthik Nayak -- 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 v4 01/10] ref-filter: add option to align atoms to the left
On Sun, Jul 26, 2015 at 12:36 AM, Karthik Nayak karthik@gmail.com wrote: On Sun, Jul 26, 2015 at 9:38 AM, Eric Sunshine sunsh...@sunshineco.com wrote: Also, it is helpful to reviewers if you include an interdiff at the bottom of your cover letter showing the changes from one version to another. You can generate an interdiff with git diff branchname-v4 branchname-v5, for instance. I've been working on the same branch, and that's why I didn't really provide interdiff's, and I kinda worked on the same branch again, so I wont be giving an interdiff for the next series either, but I'll keep this in mind and follow it from the forthcoming patch series. Thanks You can still provide an interdiff. It is quite likely that you can find the commit corresponding to v4 in the reflog for that branch (see git-reflog). Failing that, an easy way to recover the state at v4 is to create a new branch (from the parent of your current working branch) and apply the v4 patches which you sent to the mailing list. If Junio queued v4 in his 'pu' branch, then you can also recover from there. Once you've recovered the state of v4 using one of the methods mentioned here (or some other), you can make an interdiff when sending out v5. Reviewers do appreciate that you provide a URL to the previous version and take the time to explain in your cover letter what changed (and why). Including an interdiff is one more way to ease the review process, and is also appreciated, so it may be worthwhile to put in the effort to recover the state of v4 so that you can include an interdiff with v5. Doing so does require a bit of extra work for you, but that's work you only need to do *once*, whereas if you don't do it, then you place the burden on *each* reviewer for *each* version, which quickly adds up to a lot more work for those reviewing your submissions. -- 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 v4 01/10] ref-filter: add option to align atoms to the left
Karthik Nayak karthik@gmail.com writes:
- if (!ref-value) {
- populate_value(ref);
+ /*
+ * If the atom is a pseudo_atom then we re-populate the value
+ * into the ref_formatting_state stucture.
+ */
+ if (!ref-value || ref-value[atom].pseudo_atom) {
+ populate_value(state, ref);
fill_missing_values(ref-value);
I am not sure why you need to do this. populate_value() and
fill_missing_values() are fairly heavy-weight operations that are
expected to grab everything necessary from scratch, and that is why
we ensure that we do not call them more than once for each ref
with by guarding the calls with if (!ref-value).
This change is breaking that basic arrangement, and worse yet, it
forces us re-read everything about that ref, leaking old ref-value.
Why could this be a good idea?
--
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 v4 01/10] ref-filter: add option to align atoms to the left
Junio C Hamano gits...@pobox.com writes:
Karthik Nayak karthik@gmail.com writes:
-if (!ref-value) {
-populate_value(ref);
+/*
+ * If the atom is a pseudo_atom then we re-populate the value
+ * into the ref_formatting_state stucture.
+ */
+if (!ref-value || ref-value[atom].pseudo_atom) {
+populate_value(state, ref);
fill_missing_values(ref-value);
I am not sure why you need to do this. populate_value() and
fill_missing_values() are fairly heavy-weight operations that are
expected to grab everything necessary from scratch, and that is why
we ensure that we do not call them more than once for each ref
with by guarding the calls with if (!ref-value).
This change is breaking that basic arrangement, and worse yet, it
forces us re-read everything about that ref, leaking old ref-value.
Why could this be a good idea?
I think populate_value() should not take state; that is the root
cause of this mistake.
The flow should be:
- verify_format() looks at the format string and enumerates all
atoms that will ever be used in the output by calling
parse_atom() and letting it to fill used_atom[];
- when ref-value is not yet populated, populate_value() is
called, just once. This uses the enumeration in used_atom[]
and stores computed value to refs-value[atom];
- show_ref() repeatedly calls find_next() to find the next
reference to %(...), emits everything before it, and then
uses the atom value (i.e. ref-value[atom]).
I would expect that the atom value for pseudos like color and align
to be parsed and stored in ref-value in populate_value() when it is
called for the first time for each ref _just once_.
color:blue may choose to store blue as v-s, and align:4 may
choose to do v-ul = 4.
And the code that uses these values should look more like:
for (cp = format; *cp (sp = find_next(cp)); cp = ep + 1) {
struct atom_value *atomv;
ep = strchr(sp, ')');
if (cp sp)
emit(cp, sp);
get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep),
atomv);
if (atomv-is_pseudo)
apply_pseudo_state(state, atomv);
else
print_value(state, atomv);
}
where apply_pseudo_state() would switch on what kind of pseudo the
atom is and update the state accordingly, i.e. the state munging
code you added to populate_value() in this patch.
--
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 v4 01/10] ref-filter: add option to align atoms to the left
On Sat, Jul 25, 2015 at 4:30 AM, Junio C Hamano gits...@pobox.com wrote:
Junio C Hamano gits...@pobox.com writes:
Karthik Nayak karthik@gmail.com writes:
-if (!ref-value) {
-populate_value(ref);
+/*
+ * If the atom is a pseudo_atom then we re-populate the value
+ * into the ref_formatting_state stucture.
+ */
+if (!ref-value || ref-value[atom].pseudo_atom) {
+populate_value(state, ref);
fill_missing_values(ref-value);
I am not sure why you need to do this. populate_value() and
fill_missing_values() are fairly heavy-weight operations that are
expected to grab everything necessary from scratch, and that is why
we ensure that we do not call them more than once for each ref
with by guarding the calls with if (!ref-value).
This change is breaking that basic arrangement, and worse yet, it
forces us re-read everything about that ref, leaking old ref-value.
Why could this be a good idea?
This was required as populate_value() would fill the 'state' but the 'state'
being a static variable would be lost before printing and hence we would
not have the correct values of the 'state' when printing.
I think populate_value() should not take state; that is the root
cause of this mistake.
Yes! agreed. atomv should be a better candidate to hold the formatting values.
The flow should be:
- verify_format() looks at the format string and enumerates all
atoms that will ever be used in the output by calling
parse_atom() and letting it to fill used_atom[];
- when ref-value is not yet populated, populate_value() is
called, just once. This uses the enumeration in used_atom[]
and stores computed value to refs-value[atom];
- show_ref() repeatedly calls find_next() to find the next
reference to %(...), emits everything before it, and then
uses the atom value (i.e. ref-value[atom]).
I would expect that the atom value for pseudos like color and align
to be parsed and stored in ref-value in populate_value() when it is
called for the first time for each ref _just once_.
color:blue may choose to store blue as v-s, and align:4 may
choose to do v-ul = 4.
And the code that uses these values should look more like:
for (cp = format; *cp (sp = find_next(cp)); cp = ep + 1) {
struct atom_value *atomv;
ep = strchr(sp, ')');
if (cp sp)
emit(cp, sp);
get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep),
atomv);
if (atomv-is_pseudo)
apply_pseudo_state(state, atomv);
else
print_value(state, atomv);
}
where apply_pseudo_state() would switch on what kind of pseudo the
atom is and update the state accordingly, i.e. the state munging
code you added to populate_value() in this patch.
This makes more sense, and avoids the repetitive call to populate_value().
Will implement this.
Thanks
--
Regards,
Karthik Nayak
--
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
