Re: [PATCH 03/30] merge-recursive: Add explanation for src_entry and dst_entry
Elijah Newren writes:
> If I have to walk through the debugger and inspect the values found in
> here in order to figure out their meaning, despite having known these
> things inside and out some years back, then they probably need a comment
> for the casual reader to explain their purpose.
>
> Signed-off-by: Elijah Newren
> ---
> merge-recursive.c | 22 ++
> 1 file changed, 22 insertions(+)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 52521faf09..3526c8d0b8 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -513,6 +513,28 @@ static void record_df_conflict_files(struct
> merge_options *o,
>
> struct rename {
> struct diff_filepair *pair;
> + /*
> + * Because I keep forgetting every few years what src_entry and
> + * dst_entry are and have to walk through a debugger and puzzle
> + * through it to remind myself...
Very much appreciated. I recall having trouble reasoning about
them myself, too (even though I admit I wasn't involved in the
implementation of this part very much and know this code a lot less
intimately than you do in the first place).
> + *
> + * If 'before' is renamed to 'after' then src_entry will contain
> + * the versions of 'before' from the merge_base, HEAD, and MERGE in
> + * stages 1, 2, and 3; dst_entry will contain the versions of
> + * 'after' from the merge_base, HEAD, and MERGE in stages 1, 2, and
> + * 3. Thus, we have a total of six modes and oids, though some
> + * will be null. (Stage 0 is ignored; we're interested in handling
> + * conflicts.)
> + *
> + * Since we don't turn on break-rewrites by default, neither
> + * src_entry nor dst_entry can have all three of their stages have
> + * non-null oids, meaning at most four of the six will be non-null.
> + * Also, since this is a rename, both src_entry and dst_entry will
> + * have at least one non-null oid, meaning at least two will be
> + * non-null. Of the six oids, a typical rename will have three be
> + * non-null. Only two implies a rename/delete, and four implies a
> + * rename/add.
> + */
> struct stage_data *src_entry;
> struct stage_data *dst_entry;
> unsigned processed:1;
Re: [PATCH 03/30] merge-recursive: Add explanation for src_entry and dst_entry
On Mon, Nov 13, 2017 at 2:57 PM, Elijah Newren wrote: > > Perhaps: > > If 'before' is renamed to 'after' then src_entry will contain > the versions of 'before' from the merge_base, HEAD, and MERGE in > stages 1, 2, and 3; and dst_entry will contain the respective versions of > 'after' in corresponding locations. Thus, we have a total of six modes > and oids, though some will be null. (Stage 0 is ignored; we're interested > in handling conflicts.) I find that much easier to read, though I am biased with prior knowledge now. ;)
Re: [PATCH 03/30] merge-recursive: Add explanation for src_entry and dst_entry
On Mon, Nov 13, 2017 at 1:06 PM, Stefan Beller wrote:
> On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newren wrote:
>> + /*
>> +* Because I keep forgetting every few years what src_entry and
>> +* dst_entry are and have to walk through a debugger and puzzle
>> +* through it to remind myself...
>
> This repeats the commit message; and doesn't help me understanding the
> {src/dst}_entry. (Maybe drop the first part here?) I'll read on.
Yep, I'll toss it.
>> +*
>> +* If 'before' is renamed to 'after' then src_entry will contain
>> +* the versions of 'before' from the merge_base, HEAD, and MERGE in
>> +* stages 1, 2, and 3; dst_entry will contain the versions of
>> +* 'after' from the merge_base, HEAD, and MERGE in stages 1, 2, and
>> +* 3.
>
> So src == before, dst = after; no trickery with the stages (the same
> stage number
> before and after; only the order needs to be conveyed:
> base, HEAD (ours?), MERGE (theirs?)
>
> I can understand that, so I wonder if we can phrase it to mention (base,
> HEAD, MERGE) just once.
Perhaps:
If 'before' is renamed to 'after' then src_entry will contain
the versions of 'before' from the merge_base, HEAD, and MERGE in
stages 1, 2, and 3; and dst_entry will contain the respective versions of
'after' in corresponding locations. Thus, we have a total of six modes
and oids, though some will be null. (Stage 0 is ignored; we're interested
in handling conflicts.)
?
Re: [PATCH 03/30] merge-recursive: Add explanation for src_entry and dst_entry
On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newren wrote:
> If I have to walk through the debugger and inspect the values found in
> here in order to figure out their meaning, despite having known these
> things inside and out some years back, then they probably need a comment
> for the casual reader to explain their purpose.
>
> Signed-off-by: Elijah Newren
> ---
> merge-recursive.c | 22 ++
> 1 file changed, 22 insertions(+)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 52521faf09..3526c8d0b8 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -513,6 +513,28 @@ static void record_df_conflict_files(struct
> merge_options *o,
>
> struct rename {
> struct diff_filepair *pair;
> + /*
> +* Because I keep forgetting every few years what src_entry and
> +* dst_entry are and have to walk through a debugger and puzzle
> +* through it to remind myself...
This repeats the commit message; and doesn't help me understanding the
{src/dst}_entry. (Maybe drop the first part here?) I'll read on.
> +*
> +* If 'before' is renamed to 'after' then src_entry will contain
> +* the versions of 'before' from the merge_base, HEAD, and MERGE in
> +* stages 1, 2, and 3; dst_entry will contain the versions of
> +* 'after' from the merge_base, HEAD, and MERGE in stages 1, 2, and
> +* 3.
So src == before, dst = after; no trickery with the stages (the same
stage number
before and after; only the order needs to be conveyed:
base, HEAD (ours?), MERGE (theirs?)
I can understand that, so I wonder if we can phrase it to mention (base,
HEAD, MERGE) just once.
> Thus, we have a total of six modes and oids, though some
> +* will be null. (Stage 0 is ignored; we're interested in handling
s/will be/may be/ or /can be/?
> +* conflicts.)
> +*
> +* Since we don't turn on break-rewrites by default, neither
> +* src_entry nor dst_entry can have all three of their stages have
> +* non-null oids, meaning at most four of the six will be non-null.
Oh. That explains the choice of /will be/ above. Thanks!
> +* Also, since this is a rename, both src_entry and dst_entry will
> +* have at least one non-null oid, meaning at least two will be
> +* non-null. Of the six oids, a typical rename will have three be
> +* non-null. Only two implies a rename/delete, and four implies a
> +* rename/add.
That makes sense.
Thanks,
Stefan
> +*/
> struct stage_data *src_entry;
> struct stage_data *dst_entry;
> unsigned processed:1;
> --
> 2.15.0.5.g9567be9905
>
[PATCH 03/30] merge-recursive: Add explanation for src_entry and dst_entry
If I have to walk through the debugger and inspect the values found in
here in order to figure out their meaning, despite having known these
things inside and out some years back, then they probably need a comment
for the casual reader to explain their purpose.
Signed-off-by: Elijah Newren
---
merge-recursive.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/merge-recursive.c b/merge-recursive.c
index 52521faf09..3526c8d0b8 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -513,6 +513,28 @@ static void record_df_conflict_files(struct merge_options
*o,
struct rename {
struct diff_filepair *pair;
+ /*
+* Because I keep forgetting every few years what src_entry and
+* dst_entry are and have to walk through a debugger and puzzle
+* through it to remind myself...
+*
+* If 'before' is renamed to 'after' then src_entry will contain
+* the versions of 'before' from the merge_base, HEAD, and MERGE in
+* stages 1, 2, and 3; dst_entry will contain the versions of
+* 'after' from the merge_base, HEAD, and MERGE in stages 1, 2, and
+* 3. Thus, we have a total of six modes and oids, though some
+* will be null. (Stage 0 is ignored; we're interested in handling
+* conflicts.)
+*
+* Since we don't turn on break-rewrites by default, neither
+* src_entry nor dst_entry can have all three of their stages have
+* non-null oids, meaning at most four of the six will be non-null.
+* Also, since this is a rename, both src_entry and dst_entry will
+* have at least one non-null oid, meaning at least two will be
+* non-null. Of the six oids, a typical rename will have three be
+* non-null. Only two implies a rename/delete, and four implies a
+* rename/add.
+*/
struct stage_data *src_entry;
struct stage_data *dst_entry;
unsigned processed:1;
--
2.15.0.5.g9567be9905
